diff --git a/.github/workflows/ci-linux.yaml b/.github/workflows/ci-linux.yaml index db1ae7d..8881276 100644 --- a/.github/workflows/ci-linux.yaml +++ b/.github/workflows/ci-linux.yaml @@ -30,3 +30,22 @@ jobs: # inlined here lives in `make lint`, which check depends on. - name: Check run: make check + + blackbox: + name: Black-box (tailnet tag) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Set up Go + uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + with: + go-version: "1.26.6" + + # The tagged suites degrade without a tailnet: contract needs nothing + # external, compat's state-read half needs only GitHub, and the + # bridge-login half skips. A TS_AUTHKEY secret enables that half. + - name: Black-box suites + env: + TS_AUTHKEY: ${{ secrets.TS_AUTHKEY }} + run: make test-tailnet diff --git a/Makefile b/Makefile index 7ee9a65..e3c4533 100644 --- a/Makefile +++ b/Makefile @@ -80,6 +80,13 @@ lint: check: lint build go test -race ./... +# The black-box suites: cross-version compatibility against the latest +# release, and golden output for the scripted surface. Tagged out of check +# because they download the release binary and, with TS_AUTHKEY set, log a +# bridge into the local tailnet. Run them on a dev box, not in CI. +test-tailnet: + go test -race -tags tailnet ./e2e/ + install: go install -ldflags "$(LDFLAGS)" ./cmd/aperture diff --git a/README.md b/README.md index fa3a609..0934ece 100644 --- a/README.md +++ b/README.md @@ -94,37 +94,100 @@ Each aperture process on a bridge is its own device on the tailnet. The first pr A fresh device needs one login. Set `TS_AUTHKEY` to a reusable auth key and new devices authorize without the browser; devices already registered keep their credentials on disk and never consult the key again. The key decides which tailnet a fresh device joins, so use a key from the tailnet your Aperture is on. +### Command line + +Everything the TUI configures is also a command, so an aperture setup can +live in a script, a dotfile or a unit file. + +```sh +aperture bridge add work --port 8991 # save a bridge with a fixed port +aperture bridge list +aperture bridge describe work +aperture bridge delete work # logs its machines out first +aperture endpoint add e1 --bridge work http://ai # save an endpoint under a name +aperture endpoint list +aperture endpoint delete e1 +``` + +Endpoints and bridges are addressed by name. An endpoint value that is not a +saved name is read as a URL. + +Launch an agent directly with its binary name. Aperture consumes its own +flags; the first flag it does not know, or `--`, passes the rest to the +agent verbatim. Provider and model default to your last launch, then the +first compatible pair. + +```sh +aperture codex --endpoint e1 # launch codex against e1 +aperture codex --model openai/gpt-5-mini # override the model +aperture codex -- --search # pass --search to codex +alias codex='aperture codex' # feels like codex, configures itself +``` + +`aperture serve` runs one bridge on a fixed port until it is stopped, for +client configurations that need a stable address to point at. The port comes +from the bridge or from `--port`; serve never falls back to a random port. + +```sh +aperture serve --bridge work # binds the bridge's stored port +aperture serve --bridge work --port 8992 # one-off override +``` + +A systemd user unit runs serve under a supervisor: + +```ini +# ~/.config/systemd/user/aperture-work.service +[Unit] +Description=aperture bridge work +After=network-online.target + +[Service] +ExecStart=/usr/local/bin/aperture serve --bridge work +Restart=on-failure + +[Install] +WantedBy=default.target +``` + ### Flags -| 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 | +| Flag | Shorthand | Environment | Description | +|------|-----------|-------------|-------------| +| `--version` | `-v` | | Print build version and exit | +| `--debug` | `-d` | | Print environment variables set before launching the agent | +| `--endpoint` | `-e` | `APERTURE_ENDPOINT` | Aperture URL or saved endpoint name, instead of the active one | +| `--bridge` | `-b` | `APERTURE_BRIDGE` | Connect through the bridge with this name, creating it if there is none | +| `--config` | `-c` | `APERTURE_CONFIG` | Read and write this settings file | +| `--port` | `-p` | | Fixed port for `bridge add` and `serve` | +| `--provider` | `-p` | | Provider for a client launch | +| `--model` | `-m` | | Model for a client launch | 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 +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 +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. +The single-dash long forms (`-endpoint`) still parse but are deprecated: they +print a warning and will be removed in a future release. + ### Environment variables Everything the launcher reads from the environment: | Variable | Description | |----------|-------------| -| `APERTURE_ENDPOINT` | Aperture URL to open on, instead of the saved one. `-endpoint` wins over it. | -| `APERTURE_BRIDGE` | Connect through the bridge with this name, creating it if there is none. `-bridge` wins over it. | +| `APERTURE_ENDPOINT` | Aperture URL to open on, instead of the saved one. `--endpoint` wins over it. | +| `APERTURE_BRIDGE` | Connect through the bridge with this name, creating it if there is none. `--bridge` wins over it. | +| `APERTURE_CONFIG` | Read and write this settings file. `--config` wins over it. | | `TS_AUTHKEY` | Tailscale auth key that authorizes a fresh bridge device without the browser login. Read only when a new device registers; see [Concurrent sessions](#concurrent-sessions). | | `CODEX_INSTALL_DIR` | Extra directory searched for a standalone Codex binary, matching the Codex installer's own variable. | | `CODEX_HOME` | Extra Codex home searched for the standalone install layout, matching Codex's own variable. | @@ -136,10 +199,20 @@ Everything the launcher reads from the environment: ```sh make build # build ./aperture make test # run tests +make check # lint, build, and the suite under the race detector make install # install to $GOPATH/bin make clean # remove built binary ``` +### Test suites + +`make check` is the gate CI runs: lint, build, then unit and e2e tests under +the race detector. `make test-tailnet` runs the black-box suites (cross-version +compatibility against the latest release, and golden output for the scripted +surface), in CI as the Black-box job and locally on a dev box. [docs/testing.md](docs/testing.md) covers each layer +with examples, including the e2e harness, the `-update` golden regeneration +and testing an installed binary with `APERTURE_E2E_BIN`. + ## Releasing Push a tag. The release workflow runs GoReleaser on a macOS runner: it imports the Developer ID certificate from GitHub secrets into a temporary keychain, and a build hook signs and notarizes each darwin binary before archiving, so nothing unsigned is ever published. A failed signature or a rejected notarization fails the run before upload. Required secrets: `APPLE_CERT_P12` (the base64-encoded .p12), `APPLE_CERT_PASSWORD`, `APPLE_ID` and `APPLE_ID_PASSWORD` (an app-specific password). diff --git a/cmd/aperture/main.go b/cmd/aperture/main.go index b928cf3..7848d0b 100644 --- a/cmd/aperture/main.go +++ b/cmd/aperture/main.go @@ -1,9 +1,8 @@ package main import ( - "flag" + "errors" "fmt" - "io" "log/slog" "os" "os/exec" @@ -14,6 +13,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/tailscale/aperture-cli/internal/bridges" + "github.com/tailscale/aperture-cli/internal/cli" "github.com/tailscale/aperture-cli/internal/config" "github.com/tailscale/aperture-cli/internal/profiles" "github.com/tailscale/aperture-cli/internal/tui" @@ -30,11 +30,6 @@ import ( ) var ( - 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" buildDate = "unknown" @@ -108,35 +103,6 @@ func gitDescribeInDir(dir string) string { return strings.TrimSpace(string(out)) } -// 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. @@ -144,51 +110,33 @@ func startRunLog(verbose bool) func() { // stderr is safe at both call sites. The TUI either never started or has // already given the terminal back. func reportFailure(err error) { + slog.Error("command failed", "err", err) 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) +// versionLine reports the build version with its commit and date when known. +func versionLine() string { + if buildCommit != "unknown" { + return fmt.Sprintf("%s (%s, %s)", buildVersion, buildCommit, buildDate) + } + return buildVersion } -func main() { - flag.Parse() - - if *flagVersion { - if buildCommit != "unknown" { - fmt.Printf("%s (%s, %s)\n", buildVersion, buildCommit, buildDate) - } else { - fmt.Println(buildVersion) - } - os.Exit(0) +// runTUI starts the interactive launcher, the command a bare invocation +// gets. endpoint, bridge and debug come from the command line resolved. +func runTUI(endpoint, bridge string, debug bool) error { + if debug { + config.RaiseRunLog() } - - // 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) + return err } - g.Debug = *flagDebug + g.Debug = debug // Register Claude Desktop on supported platforms (darwin, windows). profiles.RegisterIfSupported() @@ -196,28 +144,48 @@ func main() { // 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")) + start, err := config.EndpointFromFlags(g, endpoint, bridge) if err != nil { slog.Error("resolving the endpoint to open on", "err", err) - reportFailure(err) - os.Exit(1) + return err } machines := bridges.NewMachines(g.Debug) p := tea.NewProgram(tui.NewModel(g, buildVersion, machines, start)) - var exitCode int + var runErr error if _, err := p.Run(); err != nil { slog.Error("launcher error", "err", err) - reportFailure(err) - exitCode = 1 + runErr = err } if err := machines.Close(); err != nil { slog.Error("shutting down bridges", "err", err) - reportFailure(err) - exitCode = 1 + if runErr == nil { + runErr = err + } } - if exitCode != 0 { - os.Exit(exitCode) + return runErr +} + +func main() { + // The log opens before dispatch, so every run logs, headless ones too. + // The TUI still owns the terminal: nothing else writes to stderr here. + closeLog := config.StartRunLog() + defer closeLog() + slog.Info("aperture starting", "version", buildVersion, "commit", buildCommit, "pid", os.Getpid()) + if err := cli.Execute(os.Args[1:], versionLine(), runTUI); err != nil { + // A launched client's exit code is aperture's own: an alias for the + // client has to behave like the client. Its stderr already said why. + // A signaled child reports -1, which would wrap to 255. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + slog.Error("launched client failed", "err", err, "exitCode", exitErr.ExitCode()) + if code := exitErr.ExitCode(); code > 0 { + os.Exit(code) + } + os.Exit(1) + } + reportFailure(err) + os.Exit(1) } } diff --git a/cmd/aperture/main_test.go b/cmd/aperture/main_test.go deleted file mode 100644 index 7412875..0000000 --- a/cmd/aperture/main_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package main - -import ( - "flag" - "testing" -) - -// -bridge= has to beat APERTURE_BRIDGE: the documented precedence is flag over -// environment, and an empty flag is the only way a shell with the variable set -// can ask for the saved endpoint. -func TestFlagOrEnv(t *testing.T) { - t.Setenv("APERTURE_BRIDGE", "work") - cases := []struct { - name string - args []string - want string - }{ - {"absent flag reads the environment", nil, "work"}, - {"flag wins", []string{"-bridge=home"}, "home"}, - {"explicitly empty flag wins", []string{"-bridge="}, ""}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - fs := flag.NewFlagSet("aperture", flag.ContinueOnError) - fs.String("bridge", "", "") - if err := fs.Parse(tc.args); err != nil { - t.Fatal(err) - } - if got := flagOrEnv(fs, "bridge", "APERTURE_BRIDGE"); got != tc.want { - t.Errorf("flagOrEnv = %q, want %q", got, tc.want) - } - }) - } -} diff --git a/docs/adr/0007-headless-cli.md b/docs/adr/0007-headless-cli.md new file mode 100644 index 0000000..110197d --- /dev/null +++ b/docs/adr/0007-headless-cli.md @@ -0,0 +1,86 @@ +# 0007. Headless CLI with a cobra command tree + +Status: accepted +Date: 2026-10-13 +Amended 2026-10-13: the single-dash long forms stay, deprecated. Guy ruled +the flag changes strictly additive; nothing that parses today may stop +parsing. Use of one prints a deprecation warning to stderr, with no version +promised for removal. The forms apply only before a client command's name: +inside a client's tail a single-dash long flag is the client's own, since +nothing before the client command ever parsed one as aperture's, and the two +are indistinguishable. + +## Why? + +Every launch walks the TUI: pick the client, pick the provider, pick the +model. A developer who wants `alias codex='aperture codex'` cannot have it, +because aperture has no verb for "launch codex". Configuration has the same +shape: adding a bridge or an endpoint is menu navigation, so nothing about an +aperture setup can live in a script, a Makefile or a teammate's paste. The +existing `-endpoint` and `-bridge` flags resolve a destination and then still +open the TUI. The failure is not missing flags; it is that the TUI is the +only interface, and menus do not compose with a shell. + +## Decision + +1. The command tree is cobra. No arguments starts the TUI. `bridge`, + `endpoint` and `serve` are subcommand groups. Each registered client gets + a command named by its binary (`aperture codex`). +2. Flags take GNU form through pflag: `--endpoint`, `--bridge`, `--provider`, + `--model`, `--port`, `--config`, `--debug`, each with a single-letter + shorthand. The pre-cobra single-dash long forms (`-endpoint`) still + parse before a client command's name: a normalization pass rewrites them + to GNU form before cobra sees them, and each rewrite prints a deprecation + warning to stderr. Inside a client's tail a single-dash long flag belongs + to the client, and only the GNU form and the single-letter shorthands are + aperture's. They are marked deprecated in docs and source, not rejected. + The environment variables are unchanged. +3. A client command parses its own tail. Aperture consumes its known flags + wherever they appear. The first unknown flag, or `--`, starts verbatim + passthrough to the client. The parser is a pure function, not cobra. +4. Commands address endpoints and bridges by Name. An endpoint value that + parses as a URL is an inline endpoint instead. Names are unique per kind + at add time. Old settings without names load unchanged. +5. A headless launch takes provider and model from `LastLaunch` when they are + still valid for that client. Otherwise it takes the first compatible + provider and its default model. `--provider` and `--model` override. +6. `--config ` and `$APERTURE_CONFIG` select an alternate + settings.json. The schema does not change; the flag picks the file. +7. Scripted surfaces stay scriptable: data goes to stdout, diagnostics and + login links go to stderr, failures exit non-zero. The login link never + enters the run log, in CLI mode as in the TUI. + +## Consequences + +cobra and pflag join go.mod. Nothing that parsed before stops parsing: the +single-dash long forms are deprecated but accepted, and the shorthands are +additive. Each of the nine client packages grows a headless launch entry +next to its menu flow. The TUI and the CLI edit the same settings file, so +a change made in one is visible in the other, and the name-uniqueness rule +applies to both. + +## Rejected + +- stdlib `flag` with hand-rolled dispatch. Fine for the four flags today. The + grammar is ten commands plus a dynamic client set, and cobra's help, + usage and flag-error behavior would be re-implemented by hand. +- Viper. settings.json is a domain-owned schema with atomic writes and + load-time defaults. The only file feature needed is a path flag. Viper's + file, env and flag merging would put a second source of truth next to the + first, and its dependency tree is heavy for that. +- Requiring `--provider` and `--model` when more than one fits. Predictable + for scripts, hostile to the alias case, which is the common case. +- No names on endpoints. The smallest diff, and the one that has the user + typing URLs or bridge IDs every day. +- Persisted provider and model defaults in settings.json. They are launch + state, not configuration, and a global default is wrong the moment two + Apertures offer different provider sets. LastLaunch covers the common case. +- Removing the single-dash long forms at the cobra switch. The original + decision; Guy ruled it out because the break buys nothing a deprecation + notice does not. + +## Revisit when + +A client ships a flag that collides with an aperture flag and `--` proves too +awkward to escape it, or configuration grows enough knobs that file, env and +flag precedence needs a real merger. The second is Viper's actual use case. diff --git a/docs/adr/0008-persistent-bridge-fixed-port.md b/docs/adr/0008-persistent-bridge-fixed-port.md new file mode 100644 index 0000000..f75e05e --- /dev/null +++ b/docs/adr/0008-persistent-bridge-fixed-port.md @@ -0,0 +1,68 @@ +# 0008. A persistent bridge on a fixed port + +Status: accepted +Date: 2026-10-13 + +## Why? + +Developers run ts-unplug as a sidecar on a fixed port, then configure an +aperture endpoint to use that port. Two tools do one job, and the sidecar is +not the signed binary, which blocks team use cases aperture-cli's signing +covers. The fixed port is the point of the sidecar: a client config needs a +stable address to point at, and aperture's routes bind `127.0.0.1:0`, a +different port every run. + +## Decision + +1. `aperture serve --bridge ` runs one Machine and one Route on a fixed + loopback port until SIGTERM or SIGINT. `serve` is its own verb. The + `bridge` command group configures and never runs anything. +2. The port lives on the Bridge record, set by `bridge add --port`. + `serve --port` overrides it for one run. A serve with neither a stored + port nor the flag errors, naming both. A port already bound is a hard + error, never a fallback to an ephemeral port. +3. Client launches through a bridge keep binding `127.0.0.1:0` even when the + Bridge has a port. The fixed port belongs to serve, so a launch never + collides with the standing listener. +4. Serve fronts the endpoint given by `--endpoint`, a name or a URL. The + default is the well-known `http://ai`. +5. Serve is a foreground process. It installs nothing. The operator's + supervisor (a systemd user unit, launchd) owns restart. + +## Consequences + +A serve holds its Machine slot for its whole life, normally slot 1. A client +launch through the same bridge claims a sibling slot and registers a sibling +device in the admin console, which is ADR 0006 working as designed. +`bridge delete` refuses while a serve holds the slot lock, because +`Machines.Destroy` already fails on a locked slot; the error names the +conflict. One serve fronts one endpoint. Two endpoints mean two serve +processes, two slots and two ports. + +The fixed listener is loopback only, and nothing authenticates the requests +it forwards: the Route strips the client Authorization header because the +Machine's tailnet identity is the ingress auth. Every process on the host can +therefore reach the user's Aperture through a serve, and on a shared machine +so can every other user, since loopback is not per-UID. This is the same +exposure ts-unplug has, which serve replaces deliberately, and single-user +workstations are aperture's audience. A listener that demands a local token +is the revisit path if a multi-user deployment ever appears. + +## Rejected + +- A daemon mode that installs and manages a service unit. That is a daemon + lifecycle, a wire protocol and a trust boundary the CLI does not otherwise + have, the same shape ADR 0006 rejected. A sample unit in the README covers + the reported workflow. +- Keeping ts-unplug for this. Two binaries where one is unsigned, and the + signed one already knows how to run a Machine. +- A default-ephemeral serve. A random port re-creates the problem the + sidecar solves: nothing stable to point a client at. +- Bare `bridge` with flags as the serve command. One word doing two jobs, + and `aperture help bridge` would confuse them forever. + +## Revisit when + +Serve needs several endpoints on one port, which means a mux and a routing +rule, or a shared daemon is wanted for its own reasons. The second is ADR +0006's revisit condition and the two should reopen together. diff --git a/docs/specs/connection-context-map.md b/docs/specs/connection-context-map.md index c5189ee..c828f5f 100644 --- a/docs/specs/connection-context-map.md +++ b/docs/specs/connection-context-map.md @@ -17,6 +17,9 @@ The context boundaries below also describe the proposed broader event refactor. |---|---|---| | Connection Attempt | One try at reaching an Aperture from one Endpoint. Has identity, a phase, a recorded progress trail, and exactly one outcome. | The TCP connection. The persisted endpoint list. | | Endpoint | The remote Aperture the user chose and the way to it. Two kinds and no third: a DirectEndpoint the host reaches itself, a BridgeEndpoint reached through a Bridge's Machine. The kind is the type, never an empty field. | The local proxy address. Anything the CLI listens on. | +| Name | The user-chosen handle of a Bridge or an Endpoint, unique per kind. How a command line addresses one. | The ID, which is generated. The URL, which still addresses an unnamed Endpoint. | +| Endpoint Entry | One row of the saved endpoint list: an Endpoint and its Name. | The Endpoint itself, a value compared by URL and bridge alone. | +| Port | The fixed loopback port `serve` binds for a Bridge. Stored on the Bridge record; zero means no fixed port. | The ephemeral port a launch's Route binds. | | Gateway | The address a client is finally told to send requests to. The Endpoint URL when no Bridge is involved, the Route's local end when one is. | The Endpoint. Only equal to it in the direct case. | | Route | The local door to one Endpoint through one Machine: a `127.0.0.1:0` listener reverse-proxying over the Machine. | A tailnet route or subnet route. | | Bridge | The thing the user configures and sees in the picker: id, display name, last tailnet joined. Persisted. | The running tsnet node. | @@ -41,7 +44,7 @@ it for our node and for every peer in the netmap at once. | Context | Subdomain | Owns | Lives in | |---|---|---|---| -| Connection | Core | Connection Attempt, Phase, Progress, Login Link, Gateway, Route, Machine, Machines | `internal/bridges`. `internal/tui` presents and dispatches, and decides nothing. | +| Connection | Core | Connection Attempt, Phase, Progress, Login Link, Gateway, Route, Machine, Machines | `internal/bridges`. `internal/tui` and `internal/cli` present and dispatch, and decide nothing. | | Settings | Supporting | Endpoint, Bridge, persistence | `internal/config` | | Client Launch | Supporting | Per-client config and env, written from a Gateway | `internal/clients/*`, `internal/profiles` | | Tailnet | Generic, external | Nodes, login, netmap, dialing | `tsnet`, `ipn`, `ipnstate`, `client/local` | @@ -122,13 +125,14 @@ two while a login is outstanding, then to one: it exits when the state leaves | connected | `model.connected`, meaning the last fetch succeeded | `ipn.Running` | Keep `connected` for the former only. The latter is a Phase, never surfaced by that word. | | Status | `*ipnstate.Status` | what phase an Attempt is in | `Status` leaves the vocabulary. Phase is the only word for the second. | | machine | the Machine we run on the user's tailnet | the computer aperture is running on, as in `browser.go:69` on which machine's clipboard an SSH session writes to | The domain object takes the word. The computer is the host. That comment needs rewording when its file is touched. | +| `bridge` (command) | the config group: `bridge add`, `bridge delete` | running the listener | The listener is `serve` ([ADR 0008](../adr/0008-persistent-bridge-fixed-port.md)). `bridge` configures and never runs anything. | ## Stored, derived, transient | Fact | Where it lives | |---|---| -| Endpoint list, active endpoint | stored, `settings.json` | -| Bridge id, name, last tailnet | stored, `settings.json` | +| Endpoint list, endpoint names, active endpoint | stored, `settings.json` | +| Bridge id, name, port, last tailnet | stored, `settings.json` | | Machine tailnet credentials | stored by tsnet under the slot's bridge state dir, never by us | | Slot lock | a held-open file lock under `bridges/locks/`, process lifetime, file content empty | | Machine, Route | transient, process lifetime, keyed by bridge id | diff --git a/docs/specs/connection-contracts.md b/docs/specs/connection-contracts.md index 6d07646..009bca0 100644 --- a/docs/specs/connection-contracts.md +++ b/docs/specs/connection-contracts.md @@ -61,14 +61,72 @@ The tables below describe the broader pass-1 event proposal, including the explicitly deferred events. Two of the three contracts are deliberately absent, with reasons, rather than -left blank: +left blank. The API contract is now defined: the headless command line is a +service surface with scripts and aliases as its callers. | Contract | Status | Reason | |---|---|---| -| API | Absent | The CLI exposes no service surface. It has no callers, so there are no caller classes to enumerate, no authentication and no authorization rule. Its outbound calls are `GET /v1/models` on an Aperture we do not own and the in-process tsnet LocalAPI, both other people's contracts. The nearest thing we define is the `Machine` port, which is covered by the domain model's behaviours. | +| API | Defined below, 100% | The command line is the surface ([ADR 0007](../adr/0007-headless-cli.md)). Outbound calls stay other people's contracts: `GET /v1/models` on an Aperture we do not own and the in-process tsnet LocalAPI. | | DDL | Absent | No relational store. Persistence is `settings.json`, a document rewritten whole. See "Persisted facts" below for the one place the DDL rules still bite. | | Domain events | Defined below, 100% | The refactor's whole point is replacing a `func(string)` log sink with typed events, so these are the contract that exists. | +## Command line + +The command line's callers are scripts, aliases and other tools, so its +grammar and its output rules are the compatibility contract. Decisions: +[ADR 0007](../adr/0007-headless-cli.md) and +[ADR 0008](../adr/0008-persistent-bridge-fixed-port.md). + +``` +aperture [flags] the TUI, unchanged +aperture bridge add [--port N] +aperture bridge list +aperture bridge describe +aperture bridge delete +aperture endpoint add [--bridge ] +aperture endpoint list +aperture endpoint delete +aperture serve --bridge [--port N] [--endpoint ] +aperture [aperture flags] [-- client args] +``` + +Flags take GNU form through pflag: `--endpoint`, `--bridge`, `--provider`, +`--model`, `--port`, `--config`, `--debug`, each with a single-letter +shorthand. The pre-cobra single-dash long forms (`-endpoint`) still parse +before a client command's name, deprecated: a normalization pass rewrites +them there and prints a warning to stderr. Inside a client's tail a +single-dash long flag belongs to the client, and only the GNU form and the +single-letter shorthands are aperture's. `--config` and `$APERTURE_CONFIG` +select an alternate settings.json. The schema does not change. + +Rules: + +- A Name addresses an endpoint or a bridge. Resolution is name first: a + value matching a saved Name is that endpoint, anything else parses as a + URL and becomes an inline endpoint. A name containing `://` is refused at + add time. Names win over URLs because any bare host parses as a URL, so + URL-first would make names unreachable. +- A client command consumes aperture's known flags anywhere in its tail. The + first unknown flag, or `--`, starts verbatim passthrough to the client. +- Provider and model default to `LastLaunch` when still valid for the client, + then to the first compatible provider and its default model. +- Data goes to stdout. Diagnostics and login links go to stderr. A failure + exits non-zero. The CLI consumes the same events the TUI does: + `LoginRequired` arrives as a printed link and never enters the run log, so + the redaction contract above is unchanged. +- `bridge delete` is `Machines.Destroy` plus the settings removal, so it + refuses while another process holds a slot lock and names the conflict. +- `serve` binds the Bridge's Port on loopback and holds the Route until + SIGTERM or SIGINT. A port already bound is a hard error, never an + ephemeral fallback. Client launches through a bridge keep ephemeral ports. +- `endpoint delete` removes the endpoint only. A bridge it used stays until + `bridge delete`; the picker's orphan cascade is a TUI nicety, not a rule. +- Writes stay where they are today. `add` and `delete` write settings. A + launch records `LastLaunch` as the TUI does. `serve` writes nothing. +- Help output is part of the surface. Every command's `--help` shows its + common invocations as examples, and a test holds the examples to the + grammar above. + ## Domain events Delivery is the same for all six and stated once rather than repeated per row: @@ -149,6 +207,12 @@ Everything else persisted is unconditional: `Endpoint.URL`, `Endpoint.BridgeID` (empty means direct, which is a real value and not an absence), `Bridge.ID`, `Bridge.Name`. +Two optional facts join them. `Endpoint.Name` is the endpoint's command-line +handle and stays empty when the user never named it; an unnamed endpoint is +addressed by URL, so empty is a real state. `Bridge.Port` is the fixed port +`serve` binds, and zero means the bridge has none. Old files carry neither +field and load unchanged. + A Bridge's Machine identity lives outside settings, in the config directory: one state directory per Slot, `bridges/` for slot 1 and `bridges/-N` after, holding the node key tsnet wrote; and one lock file diff --git a/docs/specs/connection-domain-model.md b/docs/specs/connection-domain-model.md index 7747452..2c5c80e 100644 --- a/docs/specs/connection-domain-model.md +++ b/docs/specs/connection-domain-model.md @@ -237,9 +237,10 @@ across Attempts, so it cannot be owned by any one of them. | `tailnet` | `string` | The network joined, empty until the netmap lands and after leaving. | | `routes` | `map[string]*Route` | Keyed by target URL. | -Behaviors: `Open(ctx, emit) error`, `RouteTo(ctx, url, emit) (*Route, error)`, +Behaviors: `Open(ctx, emit) error`, `RouteTo(ctx, url, port, emit) (*Route, error)`, `LeaveTailnet(ctx, emit) error`, `Destroy(ctx, emit) error`, `Close() error`, -`Tailnet() string`. Each reports what it waits on to `emit`. +`Tailnet() string`. Each reports what it waits on to `emit`. A `port` of zero +picks a free port. Invariants: - A Route can only be created through an open Machine. `RouteTo` fails rather than starts a node. @@ -277,7 +278,9 @@ Entity, inside the Machine aggregate. The local door to one Endpoint. Behaviors: `close() error`, reached only through its Machine. Invariants: belongs to exactly one Machine and one Endpoint. Its listener is -bound to loopback only. Resolves the target against the Machine's own peer map +bound to loopback only. The port is ephemeral by default; under `serve` it is +the Bridge's fixed Port ([ADR 0008](../adr/0008-persistent-bridge-fixed-port.md)). +Resolves the target against the Machine's own peer map before dialing, never the host resolver, because the host may itself be on a tailnet with a same-named node. diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 0000000..949fbca --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,123 @@ +# Testing + +aperture-cli tests at three layers: unit tests inside each package, an e2e +suite that drives the built binary as a black box, and two black-box suites +behind the `tailnet` build tag. `make check` is the gate CI runs: lint, +build, then unit and e2e under the race detector. + +## Unit tests + +Unit tests live next to the code and test through the package's real +surface, not its internals. New functionality needs them; see +`internal/cli/launch_test.go` for the table style: + +```go +{ + name: "a single-dash long flag in the tail is the client's own", + in: []string{"-config", "x.json", "--search"}, + sel: launchSelection{}, + rest: []string{"-config", "x.json", "--search"}, +}, +``` + +Tests that touch settings point HOME at a temp dir. The config package has +`loadInto` (`internal/config/startup_test.go`) for a saved-settings fixture; +the cli package has `scratchHome` (`internal/cli/cli_test.go`). + +## The e2e harness + +`e2e/` builds the real binary once in `TestMain` and drives it. Nothing here +may touch the developer's real home, config or tailnet. The pieces: + +- `run(t, bin, env, args...)` runs one invocation and returns stdout, stderr + and the error. Use it for every non-interactive assertion. +- `spawn(t, bin, args, env)` starts the binary on a PTY and returns a + `terminal` with `waitFor`, `send`, `screen` and `waitExit`. Use it for the + TUI and for anything that needs a signal mid-run. +- `hermeticEnv(t, binDir)` builds an isolated HOME, XDG_CONFIG_HOME and PATH. +- `fakeAperture(t)` serves the `/v1/models` discovery contract, so a connect + succeeds without a tailnet. +- `installStubPi(t, binDir)` installs a stub agent binary that records its + argv and env, so a test can assert exactly what the client received. +- `installStubPiSleeper` is the same stub, but it sleeps, for signal tests. +- `waitForFile` polls for a file the stub or the binary writes. + +A headless test through the whole path: + +```go +func TestHeadlessLaunchPi(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + _, stderr, err := run(t, apertureBin, env, "pi", "--endpoint", srv.URL, "--", "--search") + if err != nil { + t.Fatalf("aperture pi: %v (%s)", err, stderr) + } + argv, _ := os.ReadFile(filepath.Join(recordDir, "argv")) + if !strings.Contains(string(argv), "--search") { + t.Errorf("stub argv = %q, want the passthrough flag in it", argv) + } +} +``` + +A TUI test through a PTY: + +```go +term := spawn(t, apertureBin, []string{"-endpoint", srv.URL}, env) +term.waitFor(t, "Which editor do you want to use?") +term.send("1") +waitForFile(t, filepath.Join(recordDir, "argv")) +term.send("q") +term.waitExit(t) +``` + +Row numbers shift with whatever clients the test box has installed. Read the +screen for the row instead of guessing, the way `compat_test.go` does with +`piRowPattern`. + +A signal test (see `e2e/launch_signal_test.go`): + +```go +cmd := exec.Command(apertureBin, "serve", "--bridge", "work") +cmd.Env = env +cmd.Start() +// wait for the slot lock, so the signal interrupts real work +cmd.Process.Signal(syscall.SIGTERM) +// expect a clean, fast exit +``` + +## The black-box suites (tailnet tag) + +`make test-tailnet` runs two suites tagged `//go:build tailnet`, kept out of +the gate because they download the latest release from GitHub and, when +`TS_AUTHKEY` is set, log a bridge into the local tailnet. CI runs them in +the Black-box job on ubuntu-latest, where the bridge-login half skips +unless a TS_AUTHKEY secret exists. + +**compat** (`e2e/compat_test.go`) downloads the latest released binary, has +it write settings, state and a bridge login, then runs this branch's binary +against all of it. This is the test that proves a real old install upgrades +cleanly. Without `TS_AUTHKEY` the bridge-login half skips. + +**contract** (`e2e/contract_test.go`) pins the scripted surface byte for +byte: the columns a script parses, the stderr a user reads, the exit codes a +supervisor checks. Goldens live in `e2e/testdata/contract/`. A deliberate +formatting change regenerates them: + +```sh +go test -tags tailnet ./e2e/ -run TestContract -update +``` + +Random output (bridge IDs) and run-specific paths (the run-log location) are +normalized by `normalizeGolden` in `e2e/blackbox.go` before comparison. + +## Running against an installed binary + +Every suite builds the binary it tests from source. To test what +`make install` or a release actually put on disk, point the harness at it: + +```sh +APERTURE_E2E_BIN=$(which aperture) make test-tailnet +``` diff --git a/e2e/blackbox.go b/e2e/blackbox.go new file mode 100644 index 0000000..0a6b007 --- /dev/null +++ b/e2e/blackbox.go @@ -0,0 +1,172 @@ +package e2e + +import ( + "archive/tar" + "archive/zip" + "bytes" + "compress/gzip" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "regexp" + "runtime" + "strings" + "testing" + "time" +) + +// releaseBinary returns the latest released aperture binary, downloading and +// caching it. The compat suite runs the released binary to produce state and +// the branch binary to consume it, so the backward-compatibility claim is +// tested against what users actually have rather than a hand-built fixture. +func releaseBinary(t *testing.T) string { + t.Helper() + tag := latestReleaseTag(t) + var name string + if runtime.GOOS == "darwin" { + name = fmt.Sprintf("aperture_%s_darwin_%s.zip", tag, runtime.GOARCH) + } else { + name = fmt.Sprintf("aperture-cli_%s_%s.tar.gz", runtime.GOOS, runtime.GOARCH) + } + + dir := filepath.Join(userCacheDir(t), "aperture-e2e-releases", tag+"-"+runtime.GOOS+"-"+runtime.GOARCH) + bin := filepath.Join(dir, "aperture") + if _, err := os.Stat(bin); err == nil { + return bin + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + url := "https://github.com/tailscale/aperture-cli/releases/download/" + tag + "/" + name + resp, err := httpClient().Get(url) + if err != nil { + t.Fatalf("downloading %s: %v", url, err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("downloading %s: %s", url, resp.Status) + } + if strings.HasSuffix(name, ".zip") { + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatal(err) + } + zr, err := zip.NewReader(bytes.NewReader(body), int64(len(body))) + if err != nil { + t.Fatal(err) + } + for _, f := range zr.File { + if filepath.Base(f.Name) != "aperture" { + continue + } + rc, err := f.Open() + if err != nil { + t.Fatal(err) + } + out, err := os.OpenFile(bin, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) + if err != nil { + rc.Close() + t.Fatal(err) + } + _, err = io.Copy(out, rc) + rc.Close() + out.Close() + if err != nil { + t.Fatal(err) + } + return bin + } + t.Fatalf("no aperture binary in %s", url) + return "" + } + gz, err := gzip.NewReader(resp.Body) + if err != nil { + t.Fatal(err) + } + tr := tar.NewReader(gz) + for { + hdr, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + t.Fatal(err) + } + if filepath.Base(hdr.Name) != "aperture" { + continue + } + out, err := os.OpenFile(bin, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o755) + if err != nil { + t.Fatal(err) + } + if _, err := io.Copy(out, tr); err != nil { + out.Close() + t.Fatal(err) + } + out.Close() + return bin + } + t.Fatalf("no aperture binary in %s", url) + return "" +} + +// latestReleaseTag resolves the latest release tag the way install.sh does: +// the redirect target of /releases/latest. +func latestReleaseTag(t *testing.T) string { + t.Helper() + req, err := http.NewRequest(http.MethodHead, "https://github.com/tailscale/aperture-cli/releases/latest", nil) + if err != nil { + t.Fatal(err) + } + client := httpClient() + client.CheckRedirect = func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("resolving the latest release: %v", err) + } + defer resp.Body.Close() + loc := resp.Header.Get("Location") + tag := loc[strings.LastIndex(loc, "/")+1:] + if tag == "" || tag == loc { + t.Fatalf("no release tag in redirect %q", loc) + } + return tag +} + +func httpClient() *http.Client { + return &http.Client{ + Timeout: 60 * time.Second, + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + }, + } +} + +func userCacheDir(t *testing.T) string { + t.Helper() + dir, err := os.UserCacheDir() + if err != nil { + t.Fatal(err) + } + return dir +} + +var ( + // bridgeIDPattern matches the generated part of a bridge ID so golden + // output can pin the shape of a table without pinning randomness. + bridgeIDPattern = regexp.MustCompile(`bridge-[0-9a-f]{6}`) + // detailsPattern matches the run-log path reportFailure prints after an + // error, which carries the run's temp directory. + detailsPattern = regexp.MustCompile(`details: .*?aperture\.log`) +) + +// normalizeGolden replaces the random parts of command output with fixed +// tokens, so contract goldens stay stable across runs. +func normalizeGolden(s string) string { + s = bridgeIDPattern.ReplaceAllString(s, "bridge-XXXXXX") + return detailsPattern.ReplaceAllString(s, "details: ") +} diff --git a/e2e/bridge_concurrency_test.go b/e2e/bridge_concurrency_test.go index 0d05878..5d2e671 100644 --- a/e2e/bridge_concurrency_test.go +++ b/e2e/bridge_concurrency_test.go @@ -44,19 +44,6 @@ func waitForState(t *testing.T, what string, timeout time.Duration, cond func() t.Fatalf("timed out waiting for %s", what) } -// apertureConfigDir recovers the aperture configuration directory from a -// hermetic environment, so the test can assert on the files the run writes. -func apertureConfigDir(t *testing.T, env []string) string { - t.Helper() - for _, e := range env { - if dir, ok := strings.CutPrefix(e, "XDG_CONFIG_HOME="); ok { - return filepath.Join(dir, "aperture") - } - } - t.Fatal("environment has no XDG_CONFIG_HOME") - return "" -} - // lockPath is the lock file for one slot of the bridge, e.g. // bridges/locks/70da61-2.lock. func lockPath(configDir, suffix string, slot int) string { diff --git a/e2e/cli_test.go b/e2e/cli_test.go new file mode 100644 index 0000000..41cd160 --- /dev/null +++ b/e2e/cli_test.go @@ -0,0 +1,132 @@ +package e2e + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// The headless command surface, through the built binary: bridges and +// endpoints round-trip, the deprecated single-dash flags warn, and --config +// selects a separate settings file. +func TestCLICommandsRoundTrip(t *testing.T) { + env := hermeticEnv(t, "") + + stdout, stderr, err := run(t, apertureBin, env, "bridge", "add", "work", "--port", "8991") + if err != nil { + t.Fatalf("bridge add: %v (%s)", err, stderr) + } + if !strings.Contains(stdout, "work") || !strings.Contains(stdout, "8991") { + t.Errorf("bridge add printed %q, want the new bridge with its port", stdout) + } + + if _, stderr, err := run(t, apertureBin, env, "endpoint", "add", "e1", "--bridge", "work", "http://ai"); err != nil { + t.Fatalf("endpoint add: %v (%s)", err, stderr) + } + + stdout, _, err = run(t, apertureBin, env, "bridge", "list") + if err != nil { + t.Fatalf("bridge list: %v", err) + } + if !strings.Contains(stdout, "work") || !strings.Contains(stdout, "8991") { + t.Errorf("bridge list = %q, want work with its port", stdout) + } + + stdout, _, err = run(t, apertureBin, env, "endpoint", "list") + if err != nil { + t.Fatalf("endpoint list: %v", err) + } + if !strings.Contains(stdout, "e1") || !strings.Contains(stdout, "work") { + t.Errorf("endpoint list = %q, want e1 through work", stdout) + } + + stdout, _, err = run(t, apertureBin, env, "bridge", "describe", "work") + if err != nil { + t.Fatalf("bridge describe: %v", err) + } + if !strings.Contains(stdout, "name: work") || !strings.Contains(stdout, "port: 8991") { + t.Errorf("bridge describe = %q, want the name and port", stdout) + } + + // The deprecated single-dash long form still parses and warns on stderr. + stdout, stderr, err = run(t, apertureBin, env, "bridge", "add", "home", "-port", "8992") + if err != nil { + t.Fatalf("bridge add with deprecated flag form: %v (%s)", err, stderr) + } + if !strings.Contains(stdout, "8992") { + t.Errorf("bridge add -port printed %q, want the port stored", stdout) + } + if !strings.Contains(stderr, "deprecated") { + t.Errorf("bridge add -port: stderr = %q, want a deprecation warning", stderr) + } + + stdout, stderr, err = run(t, apertureBin, env, "-version") + if err != nil { + t.Fatalf("-version: %v", err) + } + if !strings.Contains(stderr, "deprecated") || stdout == "" { + t.Errorf("-version: stdout=%q stderr=%q, want the version and a deprecation warning", stdout, stderr) + } + + // A bridge an endpoint uses is refused; deleting the endpoint frees it. + if _, _, err := run(t, apertureBin, env, "bridge", "delete", "work"); err == nil { + t.Error("deleted a bridge an endpoint connects through") + } + if _, stderr, err := run(t, apertureBin, env, "endpoint", "delete", "e1"); err != nil { + t.Fatalf("endpoint delete: %v (%s)", err, stderr) + } + if _, stderr, err := run(t, apertureBin, env, "bridge", "delete", "work"); err != nil { + t.Fatalf("bridge delete: %v (%s)", err, stderr) + } + stdout, _, _ = run(t, apertureBin, env, "bridge", "list") + if strings.Contains(stdout, "work") { + t.Errorf("bridge list = %q after delete, want work gone", stdout) + } +} + +func TestCLIConfigFlagSelectsTheSettingsFile(t *testing.T) { + env := hermeticEnv(t, "") + settings := filepath.Join(t.TempDir(), "elsewhere.json") + + if _, stderr, err := run(t, apertureBin, env, "--config", settings, "bridge", "add", "work"); err != nil { + t.Fatalf("bridge add: %v (%s)", err, stderr) + } + stdout, _, err := run(t, apertureBin, env, "--config", settings, "bridge", "list") + if err != nil { + t.Fatalf("bridge list: %v", err) + } + if !strings.Contains(stdout, "work") { + t.Errorf("bridge list at the override = %q, want work", stdout) + } + stdout, _, err = run(t, apertureBin, env, "bridge", "list") + if err != nil { + t.Fatalf("bridge list: %v", err) + } + if strings.Contains(stdout, "work") { + t.Errorf("bridge list at the default = %q, want the override kept separate", stdout) + } +} + +// A failed command logs its error to the run log so details: actually +// has the failure diagnosis when the operator checks it. +func TestFailedCommandLogsToRunLog(t *testing.T) { + env := hermeticEnv(t, "") + _, stderr, err := run(t, apertureBin, env, "serve", "--bridge", "ghost") + if err == nil { + t.Fatal("serve with missing bridge succeeded") + } + if !strings.Contains(stderr, "details:") { + t.Fatalf("stderr = %q, want details pointing to log file", stderr) + } + + configDir := apertureConfigDir(t, env) + logData, err := os.ReadFile(filepath.Join(configDir, "aperture.log")) + if err != nil { + t.Fatalf("reading aperture.log: %v", err) + } + logStr := string(logData) + if !strings.Contains(logStr, "command failed") || !strings.Contains(logStr, "ghost") { + t.Errorf("aperture.log = %q, want error record for failed command", logStr) + } +} diff --git a/e2e/compat_test.go b/e2e/compat_test.go new file mode 100644 index 0000000..666a620 --- /dev/null +++ b/e2e/compat_test.go @@ -0,0 +1,147 @@ +//go:build tailnet + +package e2e + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "time" +) + +// piRowPattern finds the Pi row number in a client picker's screen. +var piRowPattern = regexp.MustCompile(`\[(\d)\]\s+Pi`) + +// Cross-version compatibility, black-box: the latest RELEASED binary writes +// its state, and this branch's binary reads it. The backward-compat claim +// ("old files load unchanged") is tested against what users actually run, +// not a hand-built fixture. Run with `go test -tags tailnet ./e2e/`. +func TestCompatOldStateReadsWithNewBinary(t *testing.T) { + srv := fakeAperture(t) + old := releaseBinary(t) + + // The released binary writes old-format settings and state: a direct + // endpoint from a full connect, and a bridge created by the -bridge flag + // (which saves at flag resolution, login or not). + env := hermeticEnv(t, "") + term := spawn(t, old, []string{"-endpoint", srv.URL}, env) + term.waitFor(t, "Which editor do you want to use?") + term.send("q") + term.waitExit(t) + + bridgeTerm := spawn(t, old, []string{"-bridge", "work"}, env) + // The bridge attempt hangs at login with no credentials; give the flag + // resolution time to save the bridge, then kill the attempt. + time.Sleep(2 * time.Second) + bridgeTerm.kill() + + // This branch's binary reads every bit of it. + stdout, stderr, err := run(t, apertureBin, env, "bridge", "list") + if err != nil { + t.Fatalf("bridge list on old state: %v (%s)", err, stderr) + } + if !strings.Contains(stdout, "work") { + t.Errorf("bridge list = %q, want the bridge the release binary wrote", stdout) + } + stdout, stderr, err = run(t, apertureBin, env, "endpoint", "list") + if err != nil { + t.Fatalf("endpoint list on old state: %v (%s)", err, stderr) + } + if !strings.Contains(stdout, srv.URL) { + t.Errorf("endpoint list = %q, want the endpoint the release binary verified", stdout) + } + + // And it can launch against old state without rewriting it. + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + launchEnv := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + // Reuse the same home: hermeticEnv made a fresh one, so point the launch + // at the old-state home's config dir explicitly. + settingsHome := xdgFromEnv(t, env) + launchEnv = append(launchEnv, "XDG_CONFIG_HOME="+settingsHome) + if _, stderr, err := run(t, apertureBin, launchEnv, "pi", "--endpoint", srv.URL); err != nil { + t.Fatalf("launch on old state: %v (%s)", err, stderr) + } + if _, err := os.Stat(filepath.Join(recordDir, "argv")); err != nil { + t.Error("the client did not launch on old state") + } +} + +// xdgFromEnv recovers XDG_CONFIG_HOME from a hermetic environment. +func xdgFromEnv(t *testing.T, env []string) string { + t.Helper() + for _, e := range env { + if v, ok := strings.CutPrefix(e, "XDG_CONFIG_HOME="); ok { + return v + } + } + t.Fatal("environment has no XDG_CONFIG_HOME") + return "" +} + +// With TS_AUTHKEY, the released binary logs a bridge in on the real tailnet +// and this branch's binary launches through it reusing the slot-1 +// credentials: no login link may appear. Without the key there is no +// tailnet to prove this on, so the test skips. +func TestCompatOldBridgeLoginReusedByNewBinary(t *testing.T) { + key := os.Getenv("TS_AUTHKEY") + if key == "" { + t.Skip("TS_AUTHKEY unset: no tailnet to log a bridge into") + } + old := releaseBinary(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir, "TS_AUTHKEY="+key) + + t.Cleanup(func() { + // Log the bridge out of the tailnet so it leaves no machine behind. + _, _, _ = run(t, apertureBin, env, "bridge", "delete", "compatbridge", "--force") + }) + + // The released binary logs the bridge in and launches the stub through + // it against the tailnet's own Aperture. + term := spawn(t, old, []string{"-bridge", "compatbridge", "-endpoint", "http://ai"}, env) + term.waitFor(t, "Which editor do you want to use?") + // Row numbers shift with whatever clients the box has installed, so read + // the picker instead of guessing. + row := piRowPattern.FindStringSubmatch(term.screen()) + if row == nil { + t.Fatalf("no Pi row in the release binary's picker:\n%s", term.screen()) + } + term.send(row[1]) + // The real Aperture answers /v1/models with many providers, so the + // release walks its provider, backend and model menus before launching. + // Descend them with the first row until the stub records its argv. + argv := filepath.Join(recordDir, "argv") + deadline := time.Now().Add(60 * time.Second) + for { + if _, err := os.Stat(argv); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatalf("no launch after descending the release's menus:\n%s", term.screen()) + } + term.send("1") + time.Sleep(time.Second) + } + term.send("q") + term.waitExit(t) + + // This branch launches through the same bridge. The slot-1 state the + // release wrote must authorize without a login link. + if err := os.Remove(filepath.Join(recordDir, "argv")); err != nil { + t.Fatal(err) + } + _, stderr, err := run(t, apertureBin, env, "pi", "--bridge", "compatbridge", "--endpoint", "http://ai") + if err != nil { + t.Fatalf("launch through the old bridge: %v (%s)", err, stderr) + } + if strings.Contains(stderr, "log in") { + t.Errorf("stderr = %q, want no login link: the old login should be reused", stderr) + } + if _, err := os.Stat(filepath.Join(recordDir, "argv")); err != nil { + t.Error("the client did not launch through the old bridge") + } +} diff --git a/e2e/contract_test.go b/e2e/contract_test.go new file mode 100644 index 0000000..948bc80 --- /dev/null +++ b/e2e/contract_test.go @@ -0,0 +1,102 @@ +//go:build tailnet + +package e2e + +import ( + "flag" + "fmt" + "os" + "os/exec" + "path/filepath" + "testing" +) + +// The scripted-surface contract, pinned byte for byte: the columns a script +// parses, the stderr a user reads and the exit codes a supervisor checks. +// These goldens are what make "data goes to stdout, diagnostics to stderr, +// failures exit non-zero" an enforceable contract rather than an aspiration. +// Regenerate with `go test -tags tailnet ./e2e/ -run Contract -update`. + +var update = flag.Bool("update", false, "rewrite the contract golden files") + +// contractCase is one invocation and what the contract promises for it. +type contractCase struct { + name string + args []string + code int // expected exit code +} + +func TestContract(t *testing.T) { + cases := []struct { + name string + steps []contractCase + }{ + { + name: "bridge lifecycle", + steps: []contractCase{ + {name: "add", args: []string{"bridge", "add", "work", "--port", "8991"}}, + {name: "list", args: []string{"bridge", "list"}}, + {name: "describe", args: []string{"bridge", "describe", "work"}}, + {name: "delete", args: []string{"bridge", "delete", "work"}}, + {name: "list-after-delete", args: []string{"bridge", "list"}}, + }, + }, + { + name: "endpoint lifecycle", + steps: []contractCase{ + {name: "add", args: []string{"endpoint", "add", "e1", "http://ai"}}, + {name: "add-second", args: []string{"endpoint", "add", "e2", "http://other"}}, + {name: "list", args: []string{"endpoint", "list"}}, + {name: "delete", args: []string{"endpoint", "delete", "e2"}}, + {name: "list-after-delete", args: []string{"endpoint", "list"}}, + }, + }, + { + name: "errors", + steps: []contractCase{ + {name: "serve-no-bridge", args: []string{"serve"}, code: 1}, + {name: "describe-missing", args: []string{"bridge", "describe", "ghost"}, code: 1}, + {name: "delete-missing", args: []string{"endpoint", "delete", "ghost"}, code: 1}, + {name: "add", args: []string{"endpoint", "add", "e1", "http://x"}}, + {name: "add-duplicate", args: []string{"endpoint", "add", "e1", "http://y"}, code: 1}, + }, + }, + } + + for _, suite := range cases { + t.Run(suite.name, func(t *testing.T) { + // Each suite gets a fresh home so its steps build on each other. + suiteEnv := hermeticEnv(t, "") + for _, step := range suite.steps { + stdout, stderr, err := run(t, apertureBin, suiteEnv, step.args...) + code := 0 + if exitErr, ok := err.(*exec.ExitError); ok { + code = exitErr.ExitCode() + } else if err != nil { + t.Fatalf("%s: %v", step.name, err) + } + if code != step.code { + t.Errorf("%s: exit %d, want %d (stderr %q)", step.name, code, step.code, stderr) + } + got := fmt.Sprintf("exit %d\n-- stdout --\n%s-- stderr --\n%s", code, normalizeGolden(stdout), normalizeGolden(stderr)) + golden := filepath.Join("testdata", "contract", suite.name+"-"+step.name+".golden") + if *update { + if err := os.MkdirAll(filepath.Dir(golden), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(golden, []byte(got), 0o644); err != nil { + t.Fatal(err) + } + continue + } + want, err := os.ReadFile(golden) + if err != nil { + t.Fatalf("reading %s (run with -update to create): %v", golden, err) + } + if got != string(want) { + t.Errorf("%s output changed (-update to accept):\n--- want ---\n%s\n--- got ---\n%s", step.name, want, got) + } + } + }) + } +} diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index fbb9aac..22e5b96 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -15,8 +15,14 @@ var apertureBin string // TestMain builds the binary under test once. The tests exercise what we // ship, so they run the real build rather than recompiling main's guts -// into the test binary. +// into the test binary. APERTURE_E2E_BIN overrides the build with an +// already-installed binary, so the suite can run against `make install`'s +// output or a release. func TestMain(m *testing.M) { + if bin := os.Getenv("APERTURE_E2E_BIN"); bin != "" { + apertureBin = bin + os.Exit(m.Run()) + } dir, err := os.MkdirTemp("", "aperture-e2e") if err != nil { panic(err) @@ -107,6 +113,7 @@ func TestLaunchPi(t *testing.T) { // "q" quits only from the root menu: a clean exit here also proves the // TUI took the terminal back after the child exited. + term.waitFor(t, "[0] Quick select") term.send("q") term.waitExit(t) diff --git a/e2e/harness.go b/e2e/harness.go index b084817..d6070c3 100644 --- a/e2e/harness.go +++ b/e2e/harness.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "sync" "testing" @@ -23,6 +24,10 @@ type terminal struct { cmd *exec.Cmd ptmx *os.File + waitOnce sync.Once + waitErr error + waitDone chan struct{} + mu sync.Mutex out bytes.Buffer } @@ -36,7 +41,11 @@ func spawn(t *testing.T, bin string, args []string, env []string) *terminal { if err != nil { t.Fatalf("spawning %s: %v", bin, err) } - term := &terminal{cmd: cmd, ptmx: ptmx} + term := &terminal{ + cmd: cmd, + ptmx: ptmx, + waitDone: make(chan struct{}), + } t.Cleanup(term.kill) go func() { var buf [4096]byte @@ -55,6 +64,15 @@ func spawn(t *testing.T, bin string, args []string, env []string) *terminal { return term } +func (term *terminal) wait() error { + term.waitOnce.Do(func() { + term.waitErr = term.cmd.Wait() + close(term.waitDone) + }) + <-term.waitDone + return term.waitErr +} + // kill ends the process if it has not already exited. Terminals parked on a // screen that never exits (a bridge waiting on login) are reaped here; in // suites that end in waitExit every call is a no-op. @@ -63,7 +81,7 @@ func (term *terminal) kill() { return } _ = term.cmd.Process.Kill() - _ = term.cmd.Wait() + _ = term.wait() _ = term.ptmx.Close() } @@ -103,7 +121,7 @@ func (term *terminal) waitFor(t *testing.T, substr string) { func (term *terminal) waitExit(t *testing.T) { t.Helper() done := make(chan error, 1) - go func() { done <- term.cmd.Wait() }() + go func() { done <- term.wait() }() select { case err := <-done: term.ptmx.Close() @@ -147,7 +165,7 @@ func hermeticEnv(t *testing.T, binDir string) []string { // the login-link opener runs $BROWSER, and true is a no-op. drop := map[string]bool{ "HOME": true, "XDG_CONFIG_HOME": true, "TERM": true, "PATH": true, - "APERTURE_ENDPOINT": true, "APERTURE_BRIDGE": true, + "APERTURE_ENDPOINT": true, "APERTURE_BRIDGE": true, "APERTURE_CONFIG": true, "TS_AUTHKEY": true, "BROWSER": true, } var env []string @@ -170,6 +188,32 @@ func hermeticEnv(t *testing.T, binDir string) []string { ) } +// apertureConfigDir recovers the aperture configuration directory from a +// hermetic environment, on both Linux (~/.config/aperture) and macOS +// (~/Library/Application Support/aperture). +func apertureConfigDir(t *testing.T, env []string) string { + t.Helper() + var home, xdg string + for _, e := range env { + if dir, ok := strings.CutPrefix(e, "HOME="); ok { + home = dir + } else if dir, ok := strings.CutPrefix(e, "XDG_CONFIG_HOME="); ok { + xdg = dir + } + } + if runtime.GOOS == "darwin" && home != "" { + return filepath.Join(home, "Library", "Application Support", "aperture") + } + if xdg != "" { + return filepath.Join(xdg, "aperture") + } + if home != "" { + return filepath.Join(home, ".config", "aperture") + } + t.Fatal("environment has no HOME or XDG_CONFIG_HOME") + return "" +} + // stubPi is a stand-in for the pi binary: it records its argv and // environment, and captures the generated provider extension while it still // exists — aperture removes the extension when the child exits. @@ -196,6 +240,25 @@ func installStubPi(t *testing.T, binDir string) string { return recordDir } +// stubPiSleeper signals it is running and then execs into sleep, so a +// signal sent to its PID reaches sleep's default disposition directly +// rather than a shell's trap-after-foreground-job semantics. +const stubPiSleeper = `#!/bin/sh +echo ready > "$APERTURE_E2E_RECORD/ready" +exec sleep 30 +` + +// installStubPiSleeper writes the long-running stub into binDir and returns +// the directory it signals readiness into. +func installStubPiSleeper(t *testing.T, binDir string) string { + t.Helper() + recordDir := t.TempDir() + if err := os.WriteFile(filepath.Join(binDir, "pi"), []byte(stubPiSleeper), 0o755); err != nil { + t.Fatal(err) + } + return recordDir +} + // waitForFile blocks until path exists and is non-empty. func waitForFile(t *testing.T, path string) string { t.Helper() diff --git a/e2e/launch_signal_test.go b/e2e/launch_signal_test.go new file mode 100644 index 0000000..861df50 --- /dev/null +++ b/e2e/launch_signal_test.go @@ -0,0 +1,47 @@ +//go:build unix + +package e2e + +import ( + "os/exec" + "path/filepath" + "syscall" + "testing" + "time" +) + +// A SIGTERM sent to aperture itself must reach the client it launched too: +// aperture's own NotifyContext handler consumes the signal, so nothing +// forwards it to the child unless aperture does so explicitly. A supervisor +// or `kill` on the aperture PID that leaves the child running defeats the +// whole point of sending the signal. +func TestHeadlessLaunchForwardsSIGTERM(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPiSleeper(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + cmd := exec.Command(apertureBin, "pi", "--endpoint", srv.URL) + cmd.Env = env + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + + waitForFile(t, filepath.Join(recordDir, "ready")) + + if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatalf("SIGTERM: %v", err) + } + + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + if err == nil { + t.Error("aperture pi exited 0 on SIGTERM to a still-sleeping child, want a signal-killed error") + } + case <-time.After(5 * time.Second): + _ = cmd.Process.Kill() + t.Fatal("aperture pi did not stop on SIGTERM; the child kept running") + } +} diff --git a/e2e/launch_test.go b/e2e/launch_test.go new file mode 100644 index 0000000..d6c3e57 --- /dev/null +++ b/e2e/launch_test.go @@ -0,0 +1,171 @@ +package e2e + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// The headless launch through the built binary (ADR 0007): `aperture pi` +// connects to the endpoint, resolves provider and model without a menu, and +// passes the tail to the binary verbatim. The alias case (`--search` first, +// unknown to aperture) is the passthrough rule's whole reason to exist. +func TestHeadlessLaunchPi(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + _, stderr, err := run(t, apertureBin, env, "pi", "--endpoint", srv.URL, "--", "--search") + if err != nil { + t.Fatalf("aperture pi: %v (%s)", err, stderr) + } + argv, err := os.ReadFile(filepath.Join(recordDir, "argv")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(argv), "--search") { + t.Errorf("stub argv = %q, want the passthrough flag in it", argv) + } + if !strings.Contains(string(argv), "-e\n") { + t.Errorf("stub argv = %q, want the extension loaded with -e", argv) + } + + // The alias case: no aperture flags at all, the client's flag first. + if _, stderr, err := run(t, apertureBin, env, "pi", "--endpoint", srv.URL, "--search"); err != nil { + t.Fatalf("aperture pi --search: %v (%s)", err, stderr) + } + argv, err = os.ReadFile(filepath.Join(recordDir, "argv")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(argv), "--search") { + t.Errorf("stub argv = %q for the alias case, want --search passed through", argv) + } + + // The launch recorded LastLaunch, and a named endpoint resolves by name. + if _, stderr, err := run(t, apertureBin, env, "endpoint", "add", "e1", srv.URL); err != nil { + t.Fatalf("endpoint add: %v (%s)", err, stderr) + } + if _, stderr, err := run(t, apertureBin, env, "pi", "--endpoint", "e1"); err != nil { + t.Fatalf("aperture pi --endpoint e1: %v (%s)", err, stderr) + } +} + +// $APERTURE_ENDPOINT answers when the tail names no --endpoint: the manual +// client-command parser consumes aperture's own flags itself, bypassing +// cobra's flag/environment merging, so it has to fall back to the +// environment on its own (ADR 0007 env vars). The saved active endpoint is +// seeded unreachable first, so a run that ignored the environment variable +// and fell through to it would fail rather than coincidentally succeed. +func TestHeadlessLaunchUsesEnvironmentEndpoint(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + configDir := apertureConfigDir(t, env) + if err := os.MkdirAll(configDir, 0o700); err != nil { + t.Fatal(err) + } + const unreachable = `{"endpoints":[{"url":"http://127.0.0.1:1"}]}` + if err := os.WriteFile(filepath.Join(configDir, "settings.json"), []byte(unreachable), 0o600); err != nil { + t.Fatal(err) + } + + env = append(env, "APERTURE_ENDPOINT="+srv.URL) + if _, stderr, err := run(t, apertureBin, env, "pi"); err != nil { + t.Fatalf("aperture pi with APERTURE_ENDPOINT set and no flag: %v (%s)", err, stderr) + } + if _, err := os.Stat(filepath.Join(recordDir, "argv")); err != nil { + t.Error("the client did not launch using APERTURE_ENDPOINT") + } +} + +// A launched client's exit code is aperture's own: an alias for the client +// has to behave like the client. +func TestHeadlessLaunchPropagatesTheExitCode(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + stub := filepath.Join(binDir, "pi") + if err := os.WriteFile(stub, []byte("#!/bin/sh\nexit 3\n"), 0o755); err != nil { + t.Fatal(err) + } + env := hermeticEnv(t, binDir) + + _, _, err := run(t, apertureBin, env, "pi", "--endpoint", srv.URL) + exitErr, ok := err.(*exec.ExitError) + if !ok || exitErr.ExitCode() != 3 { + t.Fatalf("exit = %v, want the client's exit code 3", err) + } +} + +// The deprecated single-dash forms belong to aperture only before the client +// command's name: inside the tail they are the client's own flags, passed +// through untouched, so nothing aperture consumes can collide with them. +func TestHeadlessLaunchPassesSingleDashClientFlags(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + if _, stderr, err := run(t, apertureBin, env, "pi", "--endpoint", srv.URL, "-config", "settings.json", "--search"); err != nil { + t.Fatalf("aperture pi with single-dash client flags: %v (%s)", err, stderr) + } + argv, err := os.ReadFile(filepath.Join(recordDir, "argv")) + if err != nil { + t.Fatal(err) + } + for _, want := range []string{"-config", "settings.json", "--search"} { + if !strings.Contains(string(argv), want) { + t.Errorf("stub argv = %q, want %q passed through", argv, want) + } + } +} + +// --version with a client command after it answers before dispatch; after +// the client's name the flag belongs to the client. +func TestVersionWithAClientCommand(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + stdout, _, err := run(t, apertureBin, env, "--version", "pi", "--endpoint", srv.URL) + if err != nil { + t.Fatalf("--version with a client after it: %v", err) + } + if strings.TrimSpace(stdout) == "" { + t.Error("--version printed nothing with a client command after it") + } + if _, err := os.Stat(filepath.Join(recordDir, "argv")); !os.IsNotExist(err) { + t.Error("the client launched despite --version before its name") + } +} + +// A non-default settings file is a trust boundary: the launch must say on +// stderr where the configuration came from, where the client is going and +// whether permission prompts are off, because a headless run has no screen +// to show any of it. +func TestHeadlessLaunchWarnsOnConfigOverride(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + settings := filepath.Join(t.TempDir(), "elsewhere.json") + + _, stderr, err := run(t, apertureBin, env, "pi", "--config", settings, "--endpoint", srv.URL) + if err != nil { + t.Fatalf("aperture pi with --config: %v (%s)", err, stderr) + } + for _, want := range []string{settings, srv.URL, "yolo mode false"} { + if !strings.Contains(stderr, want) { + t.Errorf("stderr = %q, want %q in the override warning", stderr, want) + } + } + if _, err := os.Stat(filepath.Join(recordDir, "argv")); err != nil { + t.Error("the client did not launch after the warning") + } +} diff --git a/e2e/serve_test.go b/e2e/serve_test.go new file mode 100644 index 0000000..ddc7204 --- /dev/null +++ b/e2e/serve_test.go @@ -0,0 +1,75 @@ +//go:build unix + +package e2e + +import ( + "os/exec" + "strings" + "syscall" + "testing" + "time" +) + +// The local half of serve through the built binary: resolution errors are +// immediate and a SIGTERM stops a running serve cleanly. The data plane is +// out of reach without tailnet credentials, so the route itself is asserted +// in internal/bridges with a fake node. +func TestServeResolutionErrors(t *testing.T) { + env := hermeticEnv(t, "") + + if _, stderr, err := run(t, apertureBin, env, "serve"); err == nil || !strings.Contains(stderr, "--bridge") { + t.Errorf("serve without a bridge: err=%v stderr=%q, want --bridge named", err, stderr) + } + + if _, stderr, err := run(t, apertureBin, env, "serve", "--bridge", "ghost"); err == nil || !strings.Contains(stderr, "ghost") { + t.Errorf("serve with a missing bridge: err=%v stderr=%q, want the name in the error", err, stderr) + } + + if _, stderr, err := run(t, apertureBin, env, "bridge", "add", "work"); err != nil { + t.Fatalf("bridge add: %v (%s)", err, stderr) + } + if _, stderr, err := run(t, apertureBin, env, "serve", "--bridge", "work"); err == nil || !strings.Contains(stderr, "port") { + t.Errorf("serve without a port: err=%v stderr=%q, want the port error", err, stderr) + } +} + +// A serve parked in bridge bring-up (no credentials, so it waits on a login) +// must still stop on SIGTERM, and exit zero: a supervisor restarting on +// non-zero would flap a serve the operator asked to stop. +func TestServeStopsOnSIGTERM(t *testing.T) { + env := hermeticEnv(t, "") + if _, stderr, err := run(t, apertureBin, env, "bridge", "add", "work", "--port", "8991"); err != nil { + t.Fatalf("bridge add: %v (%s)", err, stderr) + } + + cmd := exec.Command(apertureBin, "serve", "--bridge", "work") + cmd.Env = env + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + + // Wait for the slot lock: the Machine exists, so bring-up is underway + // and the signal interrupts real work rather than flag parsing. + configDir := apertureConfigDir(t, env) + waitForState(t, "the bridge's slot lock", 15*time.Second, func() bool { + _, ok := slotSuffix(configDir) + return ok + }) + + if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatalf("SIGTERM: %v", err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case err := <-done: + if err != nil { + t.Fatalf("serve exited %v on SIGTERM, want a clean stop (%s)", err, stderr.String()) + } + case <-time.After(10 * time.Second): + cmd.Process.Kill() + t.Fatal("serve ignored SIGTERM") + } +} diff --git a/e2e/testdata/contract/bridge lifecycle-add.golden b/e2e/testdata/contract/bridge lifecycle-add.golden new file mode 100644 index 0000000..4451431 --- /dev/null +++ b/e2e/testdata/contract/bridge lifecycle-add.golden @@ -0,0 +1,5 @@ +exit 0 +-- stdout -- +NAME ID PORT TAILNET +work bridge-XXXXXX 8991 - +-- stderr -- diff --git a/e2e/testdata/contract/bridge lifecycle-delete.golden b/e2e/testdata/contract/bridge lifecycle-delete.golden new file mode 100644 index 0000000..3ca7f74 --- /dev/null +++ b/e2e/testdata/contract/bridge lifecycle-delete.golden @@ -0,0 +1,3 @@ +exit 0 +-- stdout -- +-- stderr -- diff --git a/e2e/testdata/contract/bridge lifecycle-describe.golden b/e2e/testdata/contract/bridge lifecycle-describe.golden new file mode 100644 index 0000000..e299c38 --- /dev/null +++ b/e2e/testdata/contract/bridge lifecycle-describe.golden @@ -0,0 +1,7 @@ +exit 0 +-- stdout -- +name: work +id: bridge-XXXXXX +port: 8991 +machine slots on disk: none +-- stderr -- diff --git a/e2e/testdata/contract/bridge lifecycle-list-after-delete.golden b/e2e/testdata/contract/bridge lifecycle-list-after-delete.golden new file mode 100644 index 0000000..0b7ebe8 --- /dev/null +++ b/e2e/testdata/contract/bridge lifecycle-list-after-delete.golden @@ -0,0 +1,4 @@ +exit 0 +-- stdout -- +NAME ID PORT TAILNET +-- stderr -- diff --git a/e2e/testdata/contract/bridge lifecycle-list.golden b/e2e/testdata/contract/bridge lifecycle-list.golden new file mode 100644 index 0000000..4451431 --- /dev/null +++ b/e2e/testdata/contract/bridge lifecycle-list.golden @@ -0,0 +1,5 @@ +exit 0 +-- stdout -- +NAME ID PORT TAILNET +work bridge-XXXXXX 8991 - +-- stderr -- diff --git a/e2e/testdata/contract/endpoint lifecycle-add-second.golden b/e2e/testdata/contract/endpoint lifecycle-add-second.golden new file mode 100644 index 0000000..b6577ac --- /dev/null +++ b/e2e/testdata/contract/endpoint lifecycle-add-second.golden @@ -0,0 +1,5 @@ +exit 0 +-- stdout -- +NAME URL BRIDGE +e2 http://other - +-- stderr -- diff --git a/e2e/testdata/contract/endpoint lifecycle-add.golden b/e2e/testdata/contract/endpoint lifecycle-add.golden new file mode 100644 index 0000000..3b12515 --- /dev/null +++ b/e2e/testdata/contract/endpoint lifecycle-add.golden @@ -0,0 +1,5 @@ +exit 0 +-- stdout -- +NAME URL BRIDGE +e1 http://ai - +-- stderr -- diff --git a/e2e/testdata/contract/endpoint lifecycle-delete.golden b/e2e/testdata/contract/endpoint lifecycle-delete.golden new file mode 100644 index 0000000..3ca7f74 --- /dev/null +++ b/e2e/testdata/contract/endpoint lifecycle-delete.golden @@ -0,0 +1,3 @@ +exit 0 +-- stdout -- +-- stderr -- diff --git a/e2e/testdata/contract/endpoint lifecycle-list-after-delete.golden b/e2e/testdata/contract/endpoint lifecycle-list-after-delete.golden new file mode 100644 index 0000000..3b12515 --- /dev/null +++ b/e2e/testdata/contract/endpoint lifecycle-list-after-delete.golden @@ -0,0 +1,5 @@ +exit 0 +-- stdout -- +NAME URL BRIDGE +e1 http://ai - +-- stderr -- diff --git a/e2e/testdata/contract/endpoint lifecycle-list.golden b/e2e/testdata/contract/endpoint lifecycle-list.golden new file mode 100644 index 0000000..4dc80e9 --- /dev/null +++ b/e2e/testdata/contract/endpoint lifecycle-list.golden @@ -0,0 +1,6 @@ +exit 0 +-- stdout -- +NAME URL BRIDGE +e1 http://ai - +e2 http://other - +-- stderr -- diff --git a/e2e/testdata/contract/errors-add-duplicate.golden b/e2e/testdata/contract/errors-add-duplicate.golden new file mode 100644 index 0000000..ce14cc3 --- /dev/null +++ b/e2e/testdata/contract/errors-add-duplicate.golden @@ -0,0 +1,5 @@ +exit 1 +-- stdout -- +-- stderr -- +aperture: an endpoint called "e1" already exists +details: diff --git a/e2e/testdata/contract/errors-add.golden b/e2e/testdata/contract/errors-add.golden new file mode 100644 index 0000000..1a95ed8 --- /dev/null +++ b/e2e/testdata/contract/errors-add.golden @@ -0,0 +1,5 @@ +exit 0 +-- stdout -- +NAME URL BRIDGE +e1 http://x - +-- stderr -- diff --git a/e2e/testdata/contract/errors-delete-missing.golden b/e2e/testdata/contract/errors-delete-missing.golden new file mode 100644 index 0000000..5f79c59 --- /dev/null +++ b/e2e/testdata/contract/errors-delete-missing.golden @@ -0,0 +1,5 @@ +exit 1 +-- stdout -- +-- stderr -- +aperture: no endpoint called "ghost" +details: diff --git a/e2e/testdata/contract/errors-describe-missing.golden b/e2e/testdata/contract/errors-describe-missing.golden new file mode 100644 index 0000000..9066d92 --- /dev/null +++ b/e2e/testdata/contract/errors-describe-missing.golden @@ -0,0 +1,5 @@ +exit 1 +-- stdout -- +-- stderr -- +aperture: no bridge called "ghost" +details: diff --git a/e2e/testdata/contract/errors-serve-no-bridge.golden b/e2e/testdata/contract/errors-serve-no-bridge.golden new file mode 100644 index 0000000..3278bef --- /dev/null +++ b/e2e/testdata/contract/errors-serve-no-bridge.golden @@ -0,0 +1,5 @@ +exit 1 +-- stdout -- +-- stderr -- +aperture: serve needs --bridge, the name of a configured bridge +details: diff --git a/go.mod b/go.mod index 0af0e28..0b203da 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/charmbracelet/lipgloss v1.1.0 github.com/charmbracelet/x/ansi v0.11.6 github.com/creack/pty v1.1.24 + github.com/spf13/cobra v1.10.2 golang.org/x/sys v0.47.0 tailscale.com v1.102.3 ) @@ -34,6 +35,7 @@ require ( github.com/google/go-cmp v0.7.0 // indirect github.com/hdevalence/ed25519consensus v0.2.0 // indirect github.com/huin/goupnp v1.3.0 // indirect + github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/jsimonetti/rtnetlink v1.4.1 // indirect github.com/klauspost/compress v1.19.1 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect @@ -49,6 +51,7 @@ require ( github.com/pires/go-proxyproto v0.8.1 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/safchain/ethtool v0.3.0 // indirect + github.com/spf13/pflag v1.0.10 // indirect github.com/tailscale/certstore v0.1.1-0.20260409135935-3638fb84b77d // indirect github.com/tailscale/go-winio v0.0.0-20231025203758-c4f33415bf55 // indirect github.com/tailscale/hujson v0.0.0-20260302212456-ecc657c15afd // indirect diff --git a/go.sum b/go.sum index d3be899..22456c0 100644 --- a/go.sum +++ b/go.sum @@ -68,6 +68,7 @@ github.com/coder/websocket v1.8.14 h1:9L0p0iKiNOibykf283eHkKUHHrpG7f65OE3BhhO7v9 github.com/coder/websocket v1.8.14/go.mod h1:NX3SzP+inril6yawo5CQXx8+fk145lPDC6pumgx0mVg= github.com/coreos/go-iptables v0.7.1-0.20240112124308-65c67c9f46e6 h1:8h5+bWd7R6AYUslN6c6iuZWTKsKxUFDlpnmilO6R2n0= github.com/coreos/go-iptables v0.7.1-0.20240112124308-65c67c9f46e6/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFETJConOQ//Q= +github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creachadair/mds v0.25.13 h1:PsSUHV6zsfPd29k4kvm1rMoee1YFia7JyNGeMPmDcPM= github.com/creachadair/mds v0.25.13/go.mod h1:4hatI3hRM+qhzuAmqPRFvaBM8mONkS7nsLxkcuTYUIs= github.com/creachadair/msync v0.8.1 h1:QRd8si3qZ2Q4TaDL7tS/MG/lFE3YND7U7J9fy42eAFM= @@ -122,6 +123,8 @@ github.com/huin/goupnp v1.3.0 h1:UvLUlWDNpoUdYzb2TCn+MuTWtcjXKSza2n6CBdQ0xXc= github.com/huin/goupnp v1.3.0/go.mod h1:gnGPsThkYa7bFi/KWmEysQRf48l2dvR5bxr2OFckNX8= github.com/illarion/gonotify/v3 v3.0.2 h1:O7S6vcopHexutmpObkeWsnzMJt/r1hONIEogeVNmJMk= github.com/illarion/gonotify/v3 v3.0.2/go.mod h1:HWGPdPe817GfvY3w7cx6zkbzNZfi3QjcBm/wgVvEL1U= +github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= +github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/insomniacslk/dhcp v0.0.0-20240129002554-15c9b8791914 h1:kD8PseueGeYiid/Mmcv17Q0Qqicc4F46jcX22L/e/Hs= github.com/insomniacslk/dhcp v0.0.0-20240129002554-15c9b8791914/go.mod h1:3A9PQ1cunSDF/1rbTq99Ts4pVnycWg+vlPkfeD2NLFI= github.com/jellydator/ttlcache/v3 v3.1.0 h1:0gPFG0IHHP6xyUyXq+JaD8fwkDCqgqwohXNJBcYE71g= @@ -184,8 +187,14 @@ github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= +github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/safchain/ethtool v0.3.0 h1:gimQJpsI6sc1yIqP/y8GYgiXn/NjgvpM0RNoWLVVmP0= github.com/safchain/ethtool v0.3.0/go.mod h1:SA9BwrgyAqNo7M+uaL6IYbxpm5wk3L7Mm6ocLW+CJUs= +github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= +github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= +github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +github.com/spf13/pflag v1.0.10 h1:4EBh2KAYBwaONj6b2Ye1GiHfwjqyROoF4RwYO+vPwFk= +github.com/spf13/pflag v1.0.10/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/studio-b12/gowebdav v0.13.0 h1:OcwSg6IQHOFNdYHn3bPOHwSE8looG8N56Y5xTT1asqQ= github.com/studio-b12/gowebdav v0.13.0/go.mod h1:bHA7t77X/QFExdeAnDzK6vKM34kEZAcE1OX4MfiwjkE= github.com/tailscale/certstore v0.1.1-0.20260409135935-3638fb84b77d h1:JcGKBZAL7ePLwOhUdN8qGQZlP5GueEiIZwY7R62pejE= @@ -222,6 +231,7 @@ github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= go4.org/mem v0.0.0-20240501181205-ae6ca9944745 h1:Tl++JLUCe4sxGu8cTpDzRLd3tN7US4hOxG5YpKCzkek= go4.org/mem v0.0.0-20240501181205-ae6ca9944745/go.mod h1:reUoABIJ9ikfM5sgtSF3Wushcza7+WeD01VB9Lirh3g= go4.org/netipx v0.0.0-20231129151722-fdeea329fbba h1:0b9z3AuHCjxk0x/opv64kcgZLBseWJUpBw5I82+2U4M= @@ -263,6 +273,7 @@ golang.zx2c4.com/wireguard/windows v0.5.3 h1:On6j2Rpn3OEMXqBq00QEDC7bWSZrPIHKIus golang.zx2c4.com/wireguard/windows v0.5.3/go.mod h1:9TEe8TJmtwyQebdFwAkEWOPr3prrtqm+REGFifP60hI= google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gvisor.dev/gvisor v0.0.0-20260224225140-573d5e7127a8 h1:Zy8IV/+FMLxy6j6p87vk/vQGKcdnbprwjTxc8UiUtsA= gvisor.dev/gvisor v0.0.0-20260224225140-573d5e7127a8/go.mod h1:QkHjoMIBaYtpVufgwv3keYAbln78mBoCuShZrPrer1Q= honnef.co/go/tools v0.7.0 h1:w6WUp1VbkqPEgLz4rkBzH/CSU6HkoqNLp6GstyTx3lU= diff --git a/internal/bridges/attempt.go b/internal/bridges/attempt.go index afccc59..d96859a 100644 --- a/internal/bridges/attempt.go +++ b/internal/bridges/attempt.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "log/slog" - "slices" "sync/atomic" "github.com/tailscale/aperture-cli/internal/config" @@ -72,7 +71,7 @@ func BeginAttempt(g *config.Global, ep config.Endpoint, switchTailnet bool, repl a.InvalidatesActive = active.BridgeID() == bridge.ID } } - if !slices.Contains(g.Settings.Endpoints, ep) { + if !g.Settings.HasEndpoint(ep) { if err := g.UpsertEndpoint(ep); err != nil { return nil, err } @@ -101,7 +100,7 @@ func (a *Attempt) Retarget(g *config.Global, next config.Endpoint) (*Attempt, er if next == a.Endpoint { return a.Retry(), nil } - ephemeral := !slices.Contains(g.Settings.Endpoints, next) + ephemeral := !g.Settings.HasEndpoint(next) switch { case a.ephemeral: if err := g.ReplaceEndpoint(a.Endpoint, next); err != nil { @@ -200,7 +199,7 @@ func (a *Attempt) Run(ctx context.Context, machines *Machines, emit func(connect if err := mc.Open(ctx, emit); err != nil { return Gateway{}, err } - route, err := mc.RouteTo(ctx, bridged.URL(), emit) + route, err := mc.RouteTo(ctx, bridged.URL(), 0, emit) if err != nil { return Gateway{}, err } diff --git a/internal/bridges/attempt_test.go b/internal/bridges/attempt_test.go index 076b20d..86bac09 100644 --- a/internal/bridges/attempt_test.go +++ b/internal/bridges/attempt_test.go @@ -25,7 +25,7 @@ func settingsGlobal(t *testing.T, s config.Settings) (*config.Global, string) { // A failed RemoveEndpoint has to leave the attempt still ephemeral: otherwise // the candidate it wrote stays in settings and nothing will ever take it out. func TestAbandonStaysEphemeralWhenTheDropFails(t *testing.T) { - g, settingsDir := settingsGlobal(t, config.Settings{Endpoints: []config.Endpoint{config.Direct("http://ai")}}) + g, settingsDir := settingsGlobal(t, config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://ai")}}}) a, err := BeginAttempt(g, config.Direct("http://new"), false, nil) if err != nil { t.Fatal(err) @@ -63,7 +63,7 @@ func switchingAttempt(t *testing.T, logoutErr error) (*Attempt, *config.Global, t.Helper() bridge := config.Bridge{ID: "bridge-abcdef", Name: "Work", Tailnet: "corp.example.com"} ep := config.Bridged("http://ai", bridge.ID) - g, _ := settingsGlobal(t, config.Settings{Bridges: []config.Bridge{bridge}, Endpoints: []config.Endpoint{ep}}) + g, _ := settingsGlobal(t, config.Settings{Bridges: []config.Bridge{bridge}, Endpoints: []config.EndpointEntry{{Endpoint: ep}}}) a, err := BeginAttempt(g, ep, true, nil) if err != nil { t.Fatal(err) diff --git a/internal/bridges/helpers_test.go b/internal/bridges/helpers_test.go index 1f6784d..d970441 100644 --- a/internal/bridges/helpers_test.go +++ b/internal/bridges/helpers_test.go @@ -19,7 +19,7 @@ func activateMachine(ms *Machines, ctx context.Context, bridge config.Bridge, re if err := mc.Open(ctx, emit); err != nil { return "", err } - route, err := mc.RouteTo(ctx, remoteURL, emit) + route, err := mc.RouteTo(ctx, remoteURL, 0, emit) if err != nil { return "", err } diff --git a/internal/bridges/machine.go b/internal/bridges/machine.go index 8212c2a..f3bfaba 100644 --- a/internal/bridges/machine.go +++ b/internal/bridges/machine.go @@ -205,8 +205,10 @@ func (mc *Machine) Open(ctx context.Context, emit func(connection.Event)) error // RouteTo returns the Route to remoteURL through this Machine, opening one on // first use. The Machine must be open. RouteTo never starts a node to satisfy -// a Route. -func (mc *Machine) RouteTo(ctx context.Context, remoteURL string, emit func(connection.Event)) (*Route, error) { +// a Route. port is the loopback port the Route binds, zero for a free one. +// A cached Route keeps its first bind, so a fixed port works for the process +// that opens exactly one Route (ADR 0008). +func (mc *Machine) RouteTo(ctx context.Context, remoteURL string, port int, emit func(connection.Event)) (*Route, error) { target, err := parseTarget(remoteURL) if err != nil { return nil, err @@ -244,7 +246,7 @@ func (mc *Machine) RouteTo(ctx context.Context, remoteURL string, emit func(conn if route := mc.routes[key]; route != nil { return route, nil } - route, err := mc.openRoute(target) + route, err := mc.openRoute(target, port) if err != nil { return nil, err } diff --git a/internal/bridges/remove.go b/internal/bridges/remove.go index 8139229..2479438 100644 --- a/internal/bridges/remove.go +++ b/internal/bridges/remove.go @@ -24,7 +24,7 @@ func CheckRemovable(g *config.Global, bridge config.Bridge, endpoint config.Endp return errors.New("connect to another endpoint before removing the active one") } if endpoint == nil && bridge.ID != "" { - if users := endpointsThroughBridge(g, bridge.ID); len(users) > 0 { + if users := EndpointsThroughBridge(g, bridge.ID); len(users) > 0 { return fmt.Errorf("bridge %s is used by endpoint %s; remove that connection instead", bridge.Name, users[0].URL()) } } @@ -39,7 +39,7 @@ func WillDestroyMachine(g *config.Global, bridge config.Bridge, endpoint config. if bridge.ID == "" || !HasMachine(bridge.ID) { return false } - for _, other := range endpointsThroughBridge(g, bridge.ID) { + for _, other := range EndpointsThroughBridge(g, bridge.ID) { if other != endpoint { return false } @@ -147,19 +147,19 @@ func RemoveFromSettings(g *config.Global, bridge config.Bridge, endpoint config. return err } } - if bridge.ID != "" && len(endpointsThroughBridge(g, bridge.ID)) == 0 { + if bridge.ID != "" && len(EndpointsThroughBridge(g, bridge.ID)) == 0 { return g.RemoveBridge(bridge.ID) } return nil } -// endpointsThroughBridge returns every saved endpoint that connects through +// EndpointsThroughBridge returns every saved endpoint that connects through // bridgeID. -func endpointsThroughBridge(g *config.Global, bridgeID string) []config.Endpoint { +func EndpointsThroughBridge(g *config.Global, bridgeID string) []config.Endpoint { var users []config.Endpoint - for _, endpoint := range g.Settings.Endpoints { - if isReachableThroughBridge(endpoint, bridgeID) { - users = append(users, endpoint) + for _, entry := range g.Settings.Endpoints { + if isReachableThroughBridge(entry.Endpoint, bridgeID) { + users = append(users, entry.Endpoint) } } return users diff --git a/internal/bridges/route.go b/internal/bridges/route.go index 089ba0c..8a2cdb4 100644 --- a/internal/bridges/route.go +++ b/internal/bridges/route.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "net" "net/http" "net/http/httputil" @@ -55,11 +56,12 @@ func parseTarget(raw string) (*url.URL, error) { // openRoute builds the reverse proxy for one target on the Machine's node. // The proxy reports through the Machine's relay because the Route is cached // and keeps serving long after the attempt that asked for it has gone. The -// caller holds the Machine's turn. -func (mc *Machine) openRoute(target *url.URL) (*Route, error) { +// caller holds the Machine's turn. port is the loopback port to bind, zero +// for a free one; a fixed port already bound is an error, never a fallback. +func (mc *Machine) openRoute(target *url.URL, port int) (*Route, error) { node, ev := mc.node, events(mc.ev.emit) debug := mc.machines.debug - ln, err := net.Listen("tcp", "127.0.0.1:0") + ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port)) if err != nil { return nil, err } @@ -81,6 +83,7 @@ func (mc *Machine) openRoute(target *url.URL) (*Route, error) { ) elapsed := time.Since(start).Round(time.Millisecond) if err != nil { + slog.Error("bridge dial failed", "network", network, "address", address, "attempts", attempts, "elapsed", elapsed, "err", err) ev.notef("Bridge dial failed: network=%s address=%s attempts=%d elapsed=%s error=%T: %v", network, address, attempts, elapsed, err, err) return nil, err } @@ -103,6 +106,7 @@ func (mc *Machine) openRoute(target *url.URL) (*Route, error) { } proxy.Transport = transport proxy.ErrorHandler = func(w http.ResponseWriter, r *http.Request, err error) { + slog.Error("bridge proxy error", "target", redactURL(target.String()), "path", r.URL.Path, "err", err) ev.notef("Bridge proxy error: target=%s path=%s error=%T: %v", redactURL(target.String()), r.URL.Path, err, err) http.Error(w, "bridge proxy error: "+err.Error(), http.StatusBadGateway) } diff --git a/internal/bridges/route_test.go b/internal/bridges/route_test.go index 79836da..1d7edf8 100644 --- a/internal/bridges/route_test.go +++ b/internal/bridges/route_test.go @@ -2,6 +2,8 @@ package bridges import ( "context" + "fmt" + "net" "net/http" "net/http/httptest" "strings" @@ -57,3 +59,63 @@ func TestRouteDropsClientAuthorization(t *testing.T) { t.Fatalf("status = %d, want %d: Authorization crossed the Route", resp.StatusCode, http.StatusNoContent) } } + +// A serve binds the port it was given and fails when that port is taken +// (ADR 0008): a silent fallback to a free port would leave the client config +// pointing at nothing. +func TestRouteBindsAFixedPort(t *testing.T) { + held, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + free := held.Addr().(*net.TCPAddr).Port + if err := held.Close(); err != nil { + t.Fatal(err) + } + + node := &fakeNode{backendAddr: "100.64.0.2:80"} + m := NewMachines(false) + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + return node + } + defer m.Close() + bridge := config.Bridge{ID: "bridge-abcdef", Name: "Work"} + mc, err := m.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := mc.Open(context.Background(), collect(&[]string{})); err != nil { + t.Fatal(err) + } + + route, err := mc.RouteTo(context.Background(), "http://ai", free, collect(&[]string{})) + if err != nil { + t.Fatalf("RouteTo on a free fixed port: %v", err) + } + if route.LocalURL != fmt.Sprintf("http://127.0.0.1:%d", free) { + t.Errorf("LocalURL = %q, want the fixed port", route.LocalURL) + } + + // A second Machine on a taken port fails rather than falls back. + held, err = net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + taken := held.Addr().(*net.TCPAddr).Port + defer held.Close() + m2 := NewMachines(false) + m2.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + return &fakeNode{backendAddr: "100.64.0.2:80"} + } + defer m2.Close() + mc2, err := m2.For(config.Bridge{ID: "bridge-fedcba", Name: "Home"}) + if err != nil { + t.Fatal(err) + } + if err := mc2.Open(context.Background(), collect(&[]string{})); err != nil { + t.Fatal(err) + } + if _, err := mc2.RouteTo(context.Background(), "http://ai", taken, collect(&[]string{})); err == nil { + t.Error("RouteTo bound a port another listener holds") + } +} diff --git a/internal/cli/bridge.go b/internal/cli/bridge.go new file mode 100644 index 0000000..d809091 --- /dev/null +++ b/internal/cli/bridge.go @@ -0,0 +1,204 @@ +package cli + +import ( + "fmt" + "strconv" + "strings" + "text/tabwriter" + + "github.com/spf13/cobra" + "github.com/tailscale/aperture-cli/internal/bridges" + "github.com/tailscale/aperture-cli/internal/config" + "github.com/tailscale/aperture-cli/internal/connection" +) + +// bridgeCommand builds the bridge command group. The group configures +// bridges and never runs one: the listener is serve (ADR 0008). +func bridgeCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "bridge", + Short: "Configure tailnet bridges", + Example: ` aperture bridge add work --port 8991 + aperture bridge list + aperture bridge describe work + aperture bridge delete work`, + } + add := &cobra.Command{ + Use: "add ", + Short: "Add a bridge", + Example: " aperture bridge add work --port 8991", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + port, _ := cmd.Flags().GetInt("port") + b, err := g.AddBridge(args[0], port) + if err != nil { + return err + } + writeBridges(cmd, g, []config.Bridge{b}) + return nil + }, + } + add.Flags().IntP("port", "p", 0, "fixed loopback port serve binds for this bridge") + del := &cobra.Command{ + Use: "delete ", + Short: "Log out the bridge's machines and remove it", + Example: ` aperture bridge delete work + aperture bridge delete work --force`, + Long: `Log out the bridge's machines and remove it from settings. + +The logout can take a minute; the tailnet answers slowly. A bridge an +endpoint still uses, or one another aperture process is running, is +refused with the reason named. Pass --force to remove the bridge and any +endpoints connecting through it.`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + b, err := bridgeByName(g, args[0]) + if err != nil { + return err + } + force, _ := cmd.Flags().GetBool("force") + if !force { + if err := bridges.CheckRemovable(g, b, nil); err != nil { + return err + } + } + if force || bridges.WillDestroyMachine(g, b, nil) { + fmt.Fprintf(cmd.ErrOrStderr(), "logging out every machine slot of %s; this can take a minute\n", b.Name) + machines := bridges.NewMachines(g.Debug) + defer machines.Close() + if err := machines.Destroy(cmd.Context(), b, eventPrinter(cmd, g.Debug)); err != nil { + return err + } + } + if force { + for _, ep := range bridges.EndpointsThroughBridge(g, b.ID) { + _ = g.RemoveEndpoint(ep) + } + } + return g.RemoveBridge(b.ID) + }, + } + del.Flags().BoolP("force", "f", false, "force removal even if endpoints still connect through this bridge") + + cmd.AddCommand( + add, + &cobra.Command{ + Use: "list", + Short: "List the configured bridges", + Example: " aperture bridge list", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + writeBridges(cmd, g, g.Settings.Bridges) + return nil + }, + }, + &cobra.Command{ + Use: "describe ", + Short: "Show one bridge and its machine state on disk", + Example: " aperture bridge describe work", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + b, err := bridgeByName(g, args[0]) + if err != nil { + return err + } + out := cmd.OutOrStdout() + fmt.Fprintf(out, "name: %s\nid: %s\n", b.Name, b.ID) + if b.Port != 0 { + fmt.Fprintf(out, "port: %d\n", b.Port) + } + if b.Tailnet != "" { + fmt.Fprintf(out, "tailnet: %s\n", b.Tailnet) + } + slots, err := config.BridgeStateSlots(b.ID) + if err != nil { + return err + } + if len(slots) == 0 { + fmt.Fprintln(out, "machine slots on disk: none") + } else { + numbers := make([]string, 0, len(slots)) + for _, slot := range slots { + numbers = append(numbers, strconv.Itoa(slot)) + } + fmt.Fprintf(out, "machine slots on disk: %s\n", strings.Join(numbers, ", ")) + } + return nil + }, + }, + del, + ) + return cmd +} + +// bridgeByName returns the bridge called name, matched without case. Unlike +// bridge creation through the launcher it never creates: a command that +// manages bridges must not make one from a typo. +func bridgeByName(g *config.Global, name string) (config.Bridge, error) { + var matched []config.Bridge + for _, b := range g.Settings.Bridges { + if strings.EqualFold(b.Name, name) { + matched = append(matched, b) + } + } + switch len(matched) { + case 0: + return config.Bridge{}, fmt.Errorf("no bridge called %q", name) + case 1: + return matched[0], nil + } + return config.Bridge{}, fmt.Errorf("%d bridges are called %q; fix the duplicate in the settings file", len(matched), name) +} + +// writeBridges prints bridges as tabbed columns. A missing port or tailnet +// is a dash, so scripts can read fixed columns. +func writeBridges(cmd *cobra.Command, g *config.Global, list []config.Bridge) { + w := tabwriter.NewWriter(cmd.OutOrStdout(), 0, 4, 2, ' ', 0) + fmt.Fprintln(w, "NAME\tID\tPORT\tTAILNET") + for _, b := range list { + port := "-" + if b.Port != 0 { + port = strconv.Itoa(b.Port) + } + tailnet := b.Tailnet + if tailnet == "" { + tailnet = "-" + } + fmt.Fprintf(w, "%s\t%s\t%s\t%s\n", b.Name, b.ID, port, tailnet) + } + w.Flush() +} + +// eventPrinter reports a long bridge operation to the user: login links +// always, notes only under --debug. The link goes to stderr and never the +// run log, the same rule the TUI follows. +func eventPrinter(cmd *cobra.Command, debug bool) func(connection.Event) { + return func(ev connection.Event) { + switch { + case ev.Link != nil: + fmt.Fprintf(cmd.ErrOrStderr(), "log in to continue: %s\n", ev.Link.String()) + case ev.Note != "" && debug: + fmt.Fprintln(cmd.ErrOrStderr(), ev.Note) + } + } +} diff --git a/internal/cli/bridge_endpoint_test.go b/internal/cli/bridge_endpoint_test.go new file mode 100644 index 0000000..7ea2a62 --- /dev/null +++ b/internal/cli/bridge_endpoint_test.go @@ -0,0 +1,194 @@ +package cli + +import ( + "strings" + "testing" +) + +func TestBridgeCommands(t *testing.T) { + scratchHome(t) + stdout, _, err := runCLI(t, "bridge", "add", "work", "--port", "8991") + if err != nil { + t.Fatalf("add: %v", err) + } + if !strings.Contains(stdout, "work") || !strings.Contains(stdout, "8991") { + t.Errorf("add printed %q, want the new bridge with its port", stdout) + } + + // A duplicate name is refused, and the shorthand carries the port. + if _, _, err := runCLI(t, "bridge", "add", "Work"); err == nil { + t.Error("a second bridge called Work was accepted") + } + if _, _, err := runCLI(t, "bridge", "add", "home", "-p", "8992"); err != nil { + t.Fatalf("add with shorthand port: %v", err) + } + + stdout, _, err = runCLI(t, "bridge", "list") + if err != nil { + t.Fatalf("list: %v", err) + } + for _, want := range []string{"NAME", "work", "8991", "home", "8992"} { + if !strings.Contains(stdout, want) { + t.Errorf("list = %q, want %q in it", stdout, want) + } + } + + stdout, _, err = runCLI(t, "bridge", "describe", "work") + if err != nil { + t.Fatalf("describe: %v", err) + } + for _, want := range []string{"name: work", "port: 8991", "machine slots on disk: none"} { + if !strings.Contains(stdout, want) { + t.Errorf("describe = %q, want %q in it", stdout, want) + } + } + + // Delete removes a bridge that never started a machine. + if _, _, err := runCLI(t, "bridge", "delete", "work"); err != nil { + t.Fatalf("delete: %v", err) + } + stdout, _, _ = runCLI(t, "bridge", "list") + if strings.Contains(stdout, "work") { + t.Errorf("list = %q after delete, want work gone", stdout) + } + if _, _, err := runCLI(t, "bridge", "delete", "work"); err == nil { + t.Error("deleting a missing bridge did not fail") + } +} + +// A bridge an endpoint uses is refused: deleting it would strand the +// endpoint, and the error has to say why. +func TestBridgeDeleteRefusesABridgeInUse(t *testing.T) { + scratchHome(t) + if _, _, err := runCLI(t, "bridge", "add", "work"); err != nil { + t.Fatalf("bridge add: %v", err) + } + if _, _, err := runCLI(t, "endpoint", "add", "e1", "--bridge", "work", "http://ai"); err != nil { + t.Fatalf("endpoint add: %v", err) + } + if _, _, err := runCLI(t, "bridge", "delete", "work"); err == nil { + t.Error("deleted a bridge an endpoint connects through") + } +} + +func TestBridgeDeleteForceRemovesBridgeAndEndpoints(t *testing.T) { + scratchHome(t) + if _, _, err := runCLI(t, "bridge", "add", "work"); err != nil { + t.Fatalf("bridge add: %v", err) + } + if _, _, err := runCLI(t, "endpoint", "add", "e1", "--bridge", "work", "http://ai"); err != nil { + t.Fatalf("endpoint add: %v", err) + } + if _, _, err := runCLI(t, "bridge", "delete", "work"); err == nil { + t.Fatal("bridge delete without --force succeeded on bridge in use") + } + if _, _, err := runCLI(t, "bridge", "delete", "work", "--force"); err != nil { + t.Fatalf("bridge delete with --force: %v", err) + } + stdout, _, _ := runCLI(t, "bridge", "list") + if strings.Contains(stdout, "work") { + t.Errorf("bridge list = %q, want work gone", stdout) + } + stdout, _, _ = runCLI(t, "endpoint", "list") + if strings.Contains(stdout, "e1") { + t.Errorf("endpoint list = %q, want e1 gone", stdout) + } +} + +// endpoint add is a management command: a typo in --bridge must error and +// leave settings untouched, not create a bridge nobody logged in. +func TestEndpointAddRefusesAMissingBridge(t *testing.T) { + scratchHome(t) + if _, _, err := runCLI(t, "endpoint", "add", "e1", "--bridge", "workk", "http://ai"); err == nil { + t.Fatal("endpoint add created a bridge from a typo") + } + stdout, _, err := runCLI(t, "bridge", "list") + if err != nil { + t.Fatalf("bridge list: %v", err) + } + if strings.Contains(stdout, "workk") { + t.Errorf("bridge list = %q, want nothing created", stdout) + } + if strings.Contains(stdout, "workk\t") { + t.Error("unexpected tab escape") + } +} + +func TestEndpointAddPrintsCanonicalName(t *testing.T) { + scratchHome(t) + stdout, _, err := runCLI(t, "endpoint", "add", " e1 ", "http://ai") + if err != nil { + t.Fatalf("endpoint add: %v", err) + } + if strings.Contains(stdout, " e1 ") { + t.Errorf("stdout = %q, want trimmed canonical name without raw padding", stdout) + } +} + +func TestEndpointCommands(t *testing.T) { + scratchHome(t) + if _, _, err := runCLI(t, "endpoint", "add", "e1", "http://ai"); err != nil { + t.Fatalf("add: %v", err) + } + if _, _, err := runCLI(t, "endpoint", "add", "e1", "http://other"); err == nil { + t.Error("a second endpoint called e1 was accepted") + } + if _, _, err := runCLI(t, "endpoint", "add", "http://ai", "http://other"); err == nil { + t.Error("an endpoint named like a URL was accepted") + } + + stdout, _, err := runCLI(t, "endpoint", "list") + if err != nil { + t.Fatalf("list: %v", err) + } + if !strings.Contains(stdout, "e1") || !strings.Contains(stdout, "http://ai") { + t.Errorf("list = %q, want the named endpoint", stdout) + } + + // e1 names the default endpoint, which is active and undeletable, so the + // delete success path needs a second, inactive endpoint. + if _, _, err := runCLI(t, "endpoint", "add", "e2", "http://other"); err != nil { + t.Fatalf("add e2: %v", err) + } + if _, _, err := runCLI(t, "endpoint", "delete", "e2"); err != nil { + t.Fatalf("delete: %v", err) + } + stdout, _, _ = runCLI(t, "endpoint", "list") + if strings.Contains(stdout, "e2") { + t.Errorf("list = %q after delete, want e2 gone", stdout) + } + if _, _, err := runCLI(t, "endpoint", "delete", "e2"); err == nil { + t.Error("deleting a missing endpoint did not fail") + } +} + +// The active endpoint is the connection the user falls back to, so delete +// refuses it and says to connect elsewhere first. +func TestEndpointDeleteRefusesTheActiveOne(t *testing.T) { + scratchHome(t) + if _, _, err := runCLI(t, "endpoint", "add", "e1", "http://ai"); err != nil { + t.Fatalf("add: %v", err) + } + if _, _, err := runCLI(t, "endpoint", "delete", "e1"); err == nil { + t.Error("deleted the active endpoint") + } +} + +// A bridged endpoint names its bridge in the list, and naming the endpoint +// resolves it for the launcher flags. +func TestEndpointThroughABridge(t *testing.T) { + scratchHome(t) + if _, _, err := runCLI(t, "bridge", "add", "work"); err != nil { + t.Fatalf("bridge add: %v", err) + } + if _, _, err := runCLI(t, "endpoint", "add", "e1", "--bridge", "work", "http://ai"); err != nil { + t.Fatalf("endpoint add: %v", err) + } + stdout, _, err := runCLI(t, "endpoint", "list") + if err != nil { + t.Fatalf("list: %v", err) + } + if !strings.Contains(stdout, "e1") || !strings.Contains(stdout, "work") { + t.Errorf("list = %q, want e1 through work", stdout) + } +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go new file mode 100644 index 0000000..0a2218d --- /dev/null +++ b/internal/cli/cli.go @@ -0,0 +1,124 @@ +// Package cli is aperture's headless command surface: the cobra tree that +// configures settings, serves bridges and launches clients without the TUI. +// A bare invocation still starts the TUI. The grammar and its output rules +// are the command-line contract in docs/specs/connection-contracts.md. +package cli + +import ( + "fmt" + "log/slog" + "os" + + "github.com/spf13/cobra" + "github.com/tailscale/aperture-cli/internal/clients" + "github.com/tailscale/aperture-cli/internal/config" +) + +// Execute runs the command line. version is what --version prints. runTUI +// starts the interactive launcher a bare invocation gets, with the endpoint, +// bridge and debug selections resolved from flags and environment. +func Execute(args []string, version string, runTUI func(endpoint, bridge string, debug bool) error) error { + root := rootCommand(version, runTUI) + clientNames := make([]string, 0, 8) + for _, c := range clients.All(nil) { + if name := c.BinaryName(); name != "" { + clientNames = append(clientNames, name) + } + } + rewritten, deprecated := gnuForm(args, longFlags, clientNames) + warnDeprecated(root.ErrOrStderr(), deprecated) + if len(deprecated) > 0 { + slog.Warn("deprecated single-dash flag form used", "flags", deprecated) + } + // --version is answered before dispatch: a version flag with a client + // command after it would otherwise be swallowed into the client's tail. + if versionRequested(rewritten, clientNames) { + fmt.Fprintln(root.OutOrStdout(), version) + return nil + } + root.SetArgs(rewritten) + return root.Execute() +} + +// rootCommand builds the aperture command tree. +func rootCommand(version string, runTUI func(endpoint, bridge string, debug bool) error) *cobra.Command { + root := &cobra.Command{ + Use: "aperture", + Short: "Point AI coding agents at an Aperture", + Long: `aperture launches AI coding agents against an Aperture, directly or +through an embedded tailnet bridge. With no command it opens the +interactive launcher. + +The single-dash long flags (-endpoint) are deprecated and will be removed +in a future release: use the GNU form (--endpoint) or the shorthand (-e).`, + Example: ` aperture + aperture --endpoint http://ai + aperture bridge add work --port 8991 + aperture endpoint add e1 --bridge work http://ai + aperture serve --bridge work`, + SilenceErrors: true, + // The TUI owns the terminal, so usage on a root error would paint + // over nothing anyone reads. Subcommand errors keep their usage. + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + debug, _ := cmd.Flags().GetBool("debug") + return runTUI( + flagOrEnv(cmd, "endpoint", "APERTURE_ENDPOINT"), + flagOrEnv(cmd, "bridge", "APERTURE_BRIDGE"), + debug, + ) + }, + } + root.CompletionOptions.DisableDefaultCmd = true + + f := root.PersistentFlags() + f.StringP("endpoint", "e", "", "Aperture URL or saved endpoint name ($APERTURE_ENDPOINT)") + f.StringP("bridge", "b", "", "connect through the bridge with this name ($APERTURE_BRIDGE)") + f.StringP("config", "c", "", "read and write this settings file ($APERTURE_CONFIG)") + f.BoolP("debug", "d", false, "bridge diagnostics and launch environment on stderr") + root.Flags().BoolP("version", "v", false, "print version and exit") + + // The settings file must be chosen before anything reads it, and every + // subcommand reads it. Commands with DisableFlagParsing (the client + // launchers) manage their own configuration selection from the tail. + root.PersistentPreRun = func(cmd *cobra.Command, args []string) { + if cmd.DisableFlagParsing { + return + } + if path := flagOrEnv(cmd, "config", "APERTURE_CONFIG"); path != "" { + config.UseSettingsFile(path) + } + } + + root.AddCommand(bridgeCommand(), endpointCommand(), serveCommand()) + for _, c := range clients.All(nil) { + if c.BinaryName() != "" { + root.AddCommand(clientCommand(c)) + } + } + return root +} + +// flagOrEnv returns the flag's value when the flag was passed, even empty, +// and otherwise the environment variable key. The flag wins so a one-off run +// can override the shell it started in: --bridge= means no bridge. +func flagOrEnv(cmd *cobra.Command, name, key string) string { + if cmd.Flags().Changed(name) { + value, _ := cmd.Flags().GetString(name) + return value + } + return os.Getenv(key) +} + +// loadGlobal reads settings and state for a subcommand and applies --debug. +func loadGlobal(cmd *cobra.Command) (*config.Global, error) { + g, err := config.Load() + if err != nil { + return nil, err + } + g.Debug, _ = cmd.Flags().GetBool("debug") + if g.Debug { + config.RaiseRunLog() + } + return g, nil +} diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go new file mode 100644 index 0000000..036faf3 --- /dev/null +++ b/internal/cli/cli_test.go @@ -0,0 +1,120 @@ +package cli + +import ( + "bytes" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + _ "github.com/tailscale/aperture-cli/internal/clients/pi" + "github.com/tailscale/aperture-cli/internal/config" +) + +// scratchHome points the config directory at a scratch home for the whole +// test, so several invocations share one settings file. It also clears the +// process-wide settings path: UseSettingsFile survives the test that set +// it, and a repeated run (-count=2) fails on stale state without this. +func scratchHome(t *testing.T) { + t.Helper() + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) + config.UseSettingsFile("") +} + +// runCLI executes the command tree once and returns what it printed. +// runTUI must never run: these tests cover commands. +func runCLI(t *testing.T, args ...string) (stdout, stderr string, err error) { + t.Helper() + root := rootCommand("test-version", func(_, _ string, _ bool) error { + t.Fatal("a command invocation started the TUI") + return nil + }) + var out, errOut bytes.Buffer + root.SetOut(&out) + root.SetErr(&errOut) + root.SetArgs(args) + err = root.Execute() + return out.String(), errOut.String(), err +} + +// The contract makes help output part of the surface: every command shows +// example invocations, so a missing Example is a broken contract, not a +// style miss. +// --bridge= has to beat APERTURE_BRIDGE: the documented precedence is flag +// over environment, and an empty flag is the only way a shell with the +// variable set can ask for the saved endpoint. +func TestFlagOrEnv(t *testing.T) { + t.Setenv("APERTURE_BRIDGE", "work") + cases := []struct { + name string + args []string + want string + }{ + {"absent flag reads the environment", nil, "work"}, + {"flag wins", []string{"--bridge=home"}, "home"}, + {"explicitly empty flag wins", []string{"--bridge="}, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + root := rootCommand("test-version", func(_, _ string, _ bool) error { return nil }) + root.SetArgs(tc.args) + if err := root.ParseFlags(tc.args); err != nil { + t.Fatal(err) + } + if got := flagOrEnv(root, "bridge", "APERTURE_BRIDGE"); got != tc.want { + t.Errorf("flagOrEnv = %q, want %q", got, tc.want) + } + }) + } +} + +func TestClientCommandEmptyConfigOverridesEnvironment(t *testing.T) { + scratchHome(t) + t.Setenv("APERTURE_CONFIG", filepath.Join(t.TempDir(), "env.json")) + _ = Execute([]string{"pi", "--config="}, "test", func(_, _ string, _ bool) error { return nil }) + if got := config.SettingsFileOverride(); got != "" { + t.Errorf("SettingsFileOverride = %q, want empty (overridden by --config=)", got) + } +} + +func TestEveryCommandDocumentsExamples(t *testing.T) { + root := rootCommand("test-version", func(_, _ string, _ bool) error { return nil }) + var walk func(cmd *cobra.Command) + walk = func(cmd *cobra.Command) { + if cmd.Runnable() && cmd.Example == "" { + t.Errorf("%s has no examples in its help", cmd.CommandPath()) + } + for _, sub := range cmd.Commands() { + walk(sub) + } + } + walk(root) + for _, path := range []string{"aperture", "aperture bridge", "aperture endpoint"} { + cmd, _, err := root.Find(strings.Split(path, " ")[1:]) + if err != nil { + t.Fatalf("%s: %v", path, err) + } + if cmd.Example == "" { + t.Errorf("%s has no examples in its help", path) + } + } +} + +// --config selects the settings file for the command, which is what lets two +// apertures or a test keep separate configuration. +func TestConfigFlagSelectsTheSettingsFile(t *testing.T) { + scratchHome(t) + settings := filepath.Join(t.TempDir(), "elsewhere.json") + if _, _, err := runCLI(t, "--config", settings, "bridge", "add", "work"); err != nil { + t.Fatalf("bridge add: %v", err) + } + stdout, _, err := runCLI(t, "--config", settings, "bridge", "list") + if err != nil { + t.Fatalf("bridge list: %v", err) + } + if !strings.Contains(stdout, "work") { + t.Errorf("bridge list = %q, want the bridge the other invocation added", stdout) + } +} diff --git a/internal/cli/endpoint.go b/internal/cli/endpoint.go new file mode 100644 index 0000000..ed44960 --- /dev/null +++ b/internal/cli/endpoint.go @@ -0,0 +1,132 @@ +package cli + +import ( + "fmt" + "text/tabwriter" + + "github.com/spf13/cobra" + "github.com/tailscale/aperture-cli/internal/bridges" + "github.com/tailscale/aperture-cli/internal/config" +) + +// endpointCommand builds the endpoint command group. +func endpointCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "endpoint", + Short: "Configure Aperture endpoints", + Example: ` aperture endpoint add e1 --bridge work http://ai + aperture endpoint list + aperture endpoint delete e1`, + } + add := &cobra.Command{ + Use: "add ", + Short: "Save an endpoint under a name", + Example: ` aperture endpoint add e1 http://ai + aperture endpoint add e1 --bridge work http://ai`, + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + bridge := "" + if cmd.Flags().Changed("bridge") { + bridge, _ = cmd.Flags().GetString("bridge") + } + // A management command never creates a bridge: a typo must not + // leave settings holding a bridge nobody logged in. The launcher + // flags keep the creating path, which is what makes a first run + // scriptable. + location, err := config.ParseEndpointURL(args[1]) + if err != nil { + return err + } + ep := config.Endpoint(config.Direct(location)) + if bridge != "" { + b, err := bridgeByName(g, bridge) + if err != nil { + return err + } + ep = config.Bridged(location, b.ID) + } + if err := g.NameEndpoint(ep, args[0]); err != nil { + return err + } + var entry config.EndpointEntry + for _, e := range g.Settings.Endpoints { + if e.Endpoint == ep { + entry = e + break + } + } + writeEndpoints(cmd, g, []config.EndpointEntry{entry}) + return nil + }, + } + cmd.AddCommand( + add, + &cobra.Command{ + Use: "list", + Short: "List the configured endpoints", + Example: " aperture endpoint list", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + writeEndpoints(cmd, g, g.Settings.Endpoints) + return nil + }, + }, + &cobra.Command{ + Use: "delete ", + Short: "Remove a named endpoint", + Example: ` aperture endpoint delete e1 + +The active endpoint stays: connect to another one first. A bridge the +endpoint used stays until bridge delete.`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + ep, ok := g.Settings.EndpointNamed(args[0]) + if !ok { + return fmt.Errorf("no endpoint called %q", args[0]) + } + if err := bridges.CheckRemovable(g, config.Bridge{}, ep); err != nil { + return err + } + return g.RemoveEndpoint(ep) + }, + }, + ) + return cmd +} + +// writeEndpoints prints endpoint entries as tabbed columns. A missing name +// or bridge is a dash, so scripts can read fixed columns. +func writeEndpoints(cmd *cobra.Command, g *config.Global, entries []config.EndpointEntry) { + w := tabwriter.NewWriter(cmd.OutOrStdout(), 0, 4, 2, ' ', 0) + fmt.Fprintln(w, "NAME\tURL\tBRIDGE") + for _, entry := range entries { + name := entry.Name + if name == "" { + name = "-" + } + bridge := "-" + if bridged, ok := entry.Endpoint.(config.BridgeEndpoint); ok { + bridge = bridged.BridgeID() + if b, ok := g.Bridge(bridged.BridgeID()); ok { + bridge = b.Name + } + } + fmt.Fprintf(w, "%s\t%s\t%s\n", name, entry.Endpoint.URL(), bridge) + } + w.Flush() +} diff --git a/internal/cli/launch.go b/internal/cli/launch.go new file mode 100644 index 0000000..4954cf3 --- /dev/null +++ b/internal/cli/launch.go @@ -0,0 +1,220 @@ +package cli + +import ( + "context" + "fmt" + "log/slog" + "os" + "os/exec" + "os/signal" + "strconv" + "strings" + "syscall" + + "github.com/spf13/cobra" + "github.com/tailscale/aperture-cli/internal/bridges" + "github.com/tailscale/aperture-cli/internal/clients" + "github.com/tailscale/aperture-cli/internal/config" +) + +// clientCommand builds the launch command for one registered client. Flag +// parsing is the command's own: aperture consumes its known flags, and the +// first unknown flag or -- starts verbatim passthrough to the client +// (ADR 0007), so a shell alias for the binary works unchanged. +func clientCommand(c clients.Client) *cobra.Command { + binary := c.BinaryName() + return &cobra.Command{ + Use: binary + " [aperture flags] [-- client args]", + Short: "Launch " + c.Name() + " against an Aperture", + Long: `Launch ` + c.Name() + ` against an Aperture, direct or through a bridge. + +Aperture flags here are the GNU form and the single-letter shorthands: +--endpoint/-e, --bridge/-b, --provider/-p, --model/-m, --config/-c, +--debug/-d. A single-dash long flag is ` + binary + `'s own in this tail. +The first flag aperture does not know, or --, passes the rest to ` + binary + ` verbatim.`, + Example: " aperture " + binary + ` --endpoint e1 + aperture ` + binary + ` --model some-model -- --search`, + Args: cobra.ArbitraryArgs, + DisableFlagParsing: true, + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + sel, extra, err := splitLaunchArgs(args) + if err != nil { + return err + } + // DisableFlagParsing keeps the persistent pre-run from seeing + // --config, so the tail parser applies it here instead. + config.UseSettingsFile(resolvedTailFlag(sel.config, sel.configSet, "APERTURE_CONFIG")) + g, err := config.Load() + if err != nil { + return err + } + g.Debug = sel.debug + if g.Debug { + config.RaiseRunLog() + } + + endpoint := resolvedTailFlag(sel.endpoint, sel.endpointSet, "APERTURE_ENDPOINT") + bridge := resolvedTailFlag(sel.bridge, sel.bridgeSet, "APERTURE_BRIDGE") + ep, err := config.EndpointFromFlags(g, endpoint, bridge) + if err != nil { + return err + } + // A non-default settings file is a trust boundary: say where the + // configuration came from, where the client is going and whether + // its permission prompts are off, since a headless run has no + // screen to show any of it. + if path := config.SettingsFileOverride(); path != "" { + fmt.Fprintf(cmd.ErrOrStderr(), "settings from %s; connecting to %s; yolo mode %v\n", path, ep.URL(), g.Settings.YoloMode) + } + ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + defer stop() + machines := bridges.NewMachines(g.Debug) + defer func() { + if err := machines.Close(); err != nil { + slog.Error("shutting down bridges", "err", err) + } + }() + attempt, err := bridges.BeginAttempt(g, ep, false, nil) + if err != nil { + return err + } + gw, err := attempt.Run(ctx, machines, eventPrinter(cmd, g.Debug)) + if err != nil { + return err + } + if err := attempt.Commit(g, gw); err != nil { + return err + } + + spec, err := c.Spec(g, sel.provider, sel.model, extra) + if err != nil { + return err + } + child, err := spec.Command() + if err != nil { + return err + } + runErr := runChild(ctx, child) + if spec.Cleanup != nil { + spec.Cleanup() + } + return runErr + }, + } +} + +// runChild starts child and waits for it, forwarding ctx's cancellation as +// a SIGTERM so a signal sent to aperture itself reaches the client too: +// signal.NotifyContext's handler consumes the signal on aperture's process +// alone, and nothing else would propagate it to an already-running child. +// The child's own exit status is preserved either way. +func runChild(ctx context.Context, child *exec.Cmd) error { + if err := child.Start(); err != nil { + return err + } + done := make(chan error, 1) + go func() { done <- child.Wait() }() + select { + case err := <-done: + return err + case <-ctx.Done(): + if err := child.Process.Signal(syscall.SIGTERM); err != nil { + slog.Debug("could not signal child process", "err", err) + } + return <-done + } +} + +// launchSelection holds the aperture flags a client command's tail parser +// consumed. The *Set fields distinguish an explicit empty value +// (--endpoint=) from the flag being absent, so an absent flag can still +// fall back to its environment variable while an explicit empty one +// overrides it on purpose. +type launchSelection struct { + endpoint, bridge, provider, model, config string + endpointSet, bridgeSet, configSet bool + debug bool +} + +// resolvedTailFlag returns value when the tail parser saw the flag, even +// with an empty value, and otherwise the environment variable it mirrors +// (ADR 0007: $APERTURE_ENDPOINT, $APERTURE_BRIDGE, $APERTURE_CONFIG). An +// explicit empty flag overrides the environment on purpose: --bridge= +// means no bridge, not "read $APERTURE_BRIDGE instead". +func resolvedTailFlag(value string, set bool, envKey string) string { + if set { + return value + } + return os.Getenv(envKey) +} + +// splitLaunchArgs consumes aperture's known flags from a client command's +// tail. In the tail, aperture's flags are the GNU form and the single-letter +// shorthands only: a single-dash long flag is the client's own, since +// nothing before the client command ever parsed one as aperture's (ADR +// 0007). Positional tokens pass through but do not end flag consumption. +// The first unknown flag, or --, starts verbatim passthrough to the client. +func splitLaunchArgs(args []string) (launchSelection, []string, error) { + var sel launchSelection + valueFlags := map[string]*string{ + "endpoint": &sel.endpoint, "e": &sel.endpoint, + "bridge": &sel.bridge, "b": &sel.bridge, + "provider": &sel.provider, "p": &sel.provider, + "model": &sel.model, "m": &sel.model, + "config": &sel.config, "c": &sel.config, + } + setFlags := map[string]*bool{ + "endpoint": &sel.endpointSet, "e": &sel.endpointSet, + "bridge": &sel.bridgeSet, "b": &sel.bridgeSet, + "config": &sel.configSet, "c": &sel.configSet, + } + var passthrough []string + for i := 0; i < len(args); i++ { + arg := args[i] + if arg == "--" { + return sel, append(passthrough, args[i+1:]...), nil + } + if arg == "" || arg == "-" || !strings.HasPrefix(arg, "-") { + // A positional passes through; aperture keeps consuming its own + // flags past it, anywhere in the tail. + passthrough = append(passthrough, arg) + continue + } + gnu := strings.HasPrefix(arg, "--") + name, value, hasValue := strings.Cut(strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-"), "=") + if !gnu && len(name) > 1 { + // A single-dash long flag in the tail is the client's own. + return sel, append(passthrough, args[i:]...), nil + } + if name == "debug" || name == "d" { + on := true + if hasValue { + parsed, err := strconv.ParseBool(value) + if err != nil { + // Not a bool form of ours: the token belongs to the client. + return sel, append(passthrough, args[i:]...), nil + } + on = parsed + } + sel.debug = on + continue + } + target, ok := valueFlags[name] + if !ok { + return sel, append(passthrough, args[i:]...), nil + } + if !hasValue { + i++ + if i >= len(args) { + return sel, nil, fmt.Errorf("flag %s needs a value", arg) + } + value = args[i] + } + *target = value + if set, ok := setFlags[name]; ok { + *set = true + } + } + return sel, passthrough, nil +} diff --git a/internal/cli/launch_test.go b/internal/cli/launch_test.go new file mode 100644 index 0000000..561e988 --- /dev/null +++ b/internal/cli/launch_test.go @@ -0,0 +1,122 @@ +package cli + +import ( + "reflect" + "testing" +) + +func TestResolvedTailFlag(t *testing.T) { + t.Run("absent falls back to the environment", func(t *testing.T) { + t.Setenv("APERTURE_ENDPOINT", "http://env") + if got := resolvedTailFlag("", false, "APERTURE_ENDPOINT"); got != "http://env" { + t.Errorf("resolvedTailFlag = %q, want the environment value", got) + } + }) + t.Run("an explicit empty flag overrides the environment", func(t *testing.T) { + t.Setenv("APERTURE_BRIDGE", "work") + if got := resolvedTailFlag("", true, "APERTURE_BRIDGE"); got != "" { + t.Errorf("resolvedTailFlag = %q, want empty to win over the environment", got) + } + }) + t.Run("a flag value wins outright", func(t *testing.T) { + t.Setenv("APERTURE_CONFIG", "env.json") + if got := resolvedTailFlag("flag.json", true, "APERTURE_CONFIG"); got != "flag.json" { + t.Errorf("resolvedTailFlag = %q, want the flag value", got) + } + }) +} + +func TestSplitLaunchArgs(t *testing.T) { + tests := []struct { + name string + in []string + sel launchSelection + rest []string + err bool + }{ + { + name: "empty", + in: nil, + sel: launchSelection{}, + }, + { + name: "aperture flags in every form", + in: []string{"--endpoint", "e1", "--bridge=work", "-p", "openai", "-m=gpt-5", "--debug"}, + sel: launchSelection{endpoint: "e1", bridge: "work", provider: "openai", model: "gpt-5", debug: true, + endpointSet: true, bridgeSet: true}, + }, + { + name: "shorthands", + in: []string{"-e", "e1", "-b", "work", "-c", "s.json", "-d", "--search"}, + sel: launchSelection{endpoint: "e1", bridge: "work", config: "s.json", debug: true, + endpointSet: true, bridgeSet: true, configSet: true}, + rest: []string{"--search"}, + }, + { + name: "double dash starts verbatim passthrough", + in: []string{"--endpoint", "e1", "--", "--endpoint", "other"}, + sel: launchSelection{endpoint: "e1", endpointSet: true}, + rest: []string{"--endpoint", "other"}, + }, + { + name: "an explicit empty value is still set, unlike an absent flag", + in: []string{"--bridge="}, + sel: launchSelection{bridgeSet: true}, + }, + { + name: "first unknown flag starts verbatim passthrough", + in: []string{"--model", "gpt-5", "--search", "--endpoint", "e1"}, + sel: launchSelection{model: "gpt-5"}, + rest: []string{"--search", "--endpoint", "e1"}, + }, + { + name: "the alias case: a client flag arrives first", + in: []string{"--search"}, + sel: launchSelection{}, + rest: []string{"--search"}, + }, + { + name: "a positional argument passes through", + in: []string{"resume"}, + sel: launchSelection{}, + rest: []string{"resume"}, + }, + { + name: "a flag without a value is an error", + in: []string{"--endpoint"}, + err: true, + }, + { + name: "debug with a false value stays off", + in: []string{"--debug=false", "--search"}, + sel: launchSelection{}, + rest: []string{"--search"}, + }, + { + name: "debug with a junk value belongs to the client", + in: []string{"--debug=banana", "--search"}, + sel: launchSelection{}, + rest: []string{"--debug=banana", "--search"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sel, rest, err := splitLaunchArgs(tt.in) + if tt.err { + if err == nil { + t.Fatalf("splitLaunchArgs(%v) = %+v, want an error", tt.in, sel) + } + return + } + if err != nil { + t.Fatalf("splitLaunchArgs(%v): %v", tt.in, err) + } + if sel != tt.sel { + t.Errorf("splitLaunchArgs(%v) selection = %+v, want %+v", tt.in, sel, tt.sel) + } + if !reflect.DeepEqual(rest, tt.rest) { + t.Errorf("splitLaunchArgs(%v) passthrough = %v, want %v", tt.in, rest, tt.rest) + } + }) + } +} diff --git a/internal/cli/normalize.go b/internal/cli/normalize.go new file mode 100644 index 0000000..ab7573f --- /dev/null +++ b/internal/cli/normalize.go @@ -0,0 +1,132 @@ +package cli + +import ( + "fmt" + "io" + "slices" + "strconv" + "strings" +) + +// longFlags are the flag names the deprecated single-dash form may spell. +// gnuForm rewrites -name to --name for these and nothing else. +var longFlags = []string{ + "endpoint", "bridge", "provider", "model", "port", + "config", "debug", "version", "help", +} + +// valueFlags are the flags that take a value, in every spelling. A value can +// be spelled like a command name, so the scanner must skip it rather than +// mistake it for one. +var valueFlags = []string{ + "endpoint", "e", "bridge", "b", "provider", "p", "model", "m", "config", "c", "port", +} + +// tailStart returns the index where a client command's tail begins: the +// first token naming a client, or -- , or the end. A flag's value is never +// taken for a command. +// +// ponytail: a bridge named after a client binary makes tailStart stop early, +// so a deprecated single-dash flag after it errors instead of parsing. The +// fix is a real parser and this corner is not worth one. +func tailStart(args []string, clientNames []string) int { + expectingValue := false + for i, arg := range args { + if arg == "--" { + return i + } + if expectingValue { + expectingValue = false + continue + } + if slices.Contains(clientNames, arg) { + return i + } + if arg == "" || arg == "-" || !strings.HasPrefix(arg, "-") { + continue + } + name, _, hasValue := strings.Cut(strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-"), "=") + if !hasValue && slices.Contains(valueFlags, name) { + expectingValue = true + } + } + return len(args) +} + +// gnuForm rewrites the deprecated single-dash long flags (-endpoint) to the +// GNU form (--endpoint) so pflag accepts them, everywhere before a client +// command's tail. Rewriting stops at a client's name and at --: inside a +// client's tail a single-dash long flag is the client's own, and the +// deprecated forms only ever applied to aperture's own flags. The rewritten +// names come back so the caller can warn. +// +// Deprecated: the single-dash long form exists for backward compatibility +// (ADR 0007) and will be removed in a future release. +func gnuForm(args []string, long []string, clientNames []string) (rewritten, deprecated []string) { + head, tail := args[:tailStart(args, clientNames)], args[tailStart(args, clientNames):] + expectingValue := false + for _, arg := range head { + if expectingValue { + expectingValue = false + rewritten = append(rewritten, arg) + continue + } + if rest, ok := strings.CutPrefix(arg, "-"); ok && !strings.HasPrefix(rest, "-") && len(rest) > 1 { + name, value, hasValue := strings.Cut(rest, "=") + if slices.Contains(long, name) { + deprecated = append(deprecated, name) + arg = "--" + name + if hasValue { + arg += "=" + value + } + } + } + name, _, hasValue := strings.Cut(strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-"), "=") + if !hasValue && slices.Contains(valueFlags, name) { + expectingValue = true + } + rewritten = append(rewritten, arg) + } + return append(rewritten, tail...), deprecated +} + +// versionRequested reports whether --version or -v, with no value or a true +// one, appears before a client command's tail. After a client's name the +// flag belongs to the client, so `aperture --version pi` prints the version +// while `aperture pi --version` passes it through. A later flag overrides +// an earlier one, same as splitLaunchArgs' --debug. +func versionRequested(args []string, clientNames []string) bool { + requested := false + expectingValue := false + for _, arg := range args[:tailStart(args, clientNames)] { + if expectingValue { + expectingValue = false + continue + } + if !strings.HasPrefix(arg, "-") { + continue + } + name, value, hasValue := strings.Cut(strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-"), "=") + if !hasValue && slices.Contains(valueFlags, name) { + expectingValue = true + } + if name != "version" && name != "v" { + continue + } + requested = true + if hasValue { + if on, err := strconv.ParseBool(value); err == nil { + requested = on + } + } + } + return requested +} + +// warnDeprecated writes the deprecation warning for each single-dash long +// flag gnuForm rewrote. +func warnDeprecated(w io.Writer, deprecated []string) { + for _, name := range deprecated { + fmt.Fprintf(w, "warning: -%s is deprecated and will be removed in a future release; use --%s instead\n", name, name) + } +} diff --git a/internal/cli/normalize_test.go b/internal/cli/normalize_test.go new file mode 100644 index 0000000..9c353cd --- /dev/null +++ b/internal/cli/normalize_test.go @@ -0,0 +1,119 @@ +package cli + +import ( + "strings" + "testing" +) + +var testClients = []string{"pi", "codex"} + +func TestGnuForm(t *testing.T) { + tests := []struct { + name string + in []string + want []string + deprecated []string + }{ + { + name: "single-dash long flag is rewritten", + in: []string{"-endpoint", "http://ai"}, + want: []string{"--endpoint", "http://ai"}, + deprecated: []string{"endpoint"}, + }, + { + name: "attached value is kept", + in: []string{"-endpoint=http://ai"}, + want: []string{"--endpoint=http://ai"}, + deprecated: []string{"endpoint"}, + }, + { + name: "GNU form passes through", + in: []string{"--endpoint", "http://ai"}, + want: []string{"--endpoint", "http://ai"}, + }, + { + name: "shorthand is not a long flag", + in: []string{"-e", "http://ai"}, + want: []string{"-e", "http://ai"}, + }, + { + name: "unknown single-dash flag passes through", + in: []string{"-search"}, + want: []string{"-search"}, + }, + { + name: "everything after the separator is verbatim", + in: []string{"-endpoint", "http://ai", "--", "-endpoint", "-model"}, + want: []string{"--endpoint", "http://ai", "--", "-endpoint", "-model"}, + deprecated: []string{"endpoint"}, + }, + { + name: "a lone dash is not a flag", + in: []string{"-"}, + want: []string{"-"}, + }, + { + name: "a flag's value is not taken for a client name", + in: []string{"-endpoint", "pi", "-debug"}, + want: []string{"--endpoint", "pi", "--debug"}, + deprecated: []string{"endpoint", "debug"}, + }, + { + name: "rewriting stops at a client command's name", + in: []string{"-debug", "pi", "-config", "x"}, + want: []string{"--debug", "pi", "-config", "x"}, + deprecated: []string{"debug"}, + }, + { + name: "a flag value named like a deprecated flag is not rewritten", + in: []string{"--config", "-debug", "bridge", "list"}, + want: []string{"--config", "-debug", "bridge", "list"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, deprecated := gnuForm(tt.in, longFlags, testClients) + if strings.Join(got, " ") != strings.Join(tt.want, " ") { + t.Errorf("gnuForm(%v) = %v, want %v", tt.in, got, tt.want) + } + if strings.Join(deprecated, " ") != strings.Join(tt.deprecated, " ") { + t.Errorf("gnuForm(%v) deprecated = %v, want %v", tt.in, deprecated, tt.deprecated) + } + }) + } +} + +func TestVersionRequested(t *testing.T) { + tests := []struct { + name string + in []string + want bool + }{ + {name: "no version flag", in: []string{"bridge", "list"}, want: false}, + {name: "before a client name", in: []string{"--debug", "--version", "pi"}, want: true}, + {name: "after a client name it is the client's", in: []string{"pi", "--version"}, want: false}, + {name: "attached value form", in: []string{"--version=true", "pi"}, want: true}, + {name: "a flag value is not a client name", in: []string{"--endpoint", "pi", "-v"}, want: true}, + {name: "the bare shorthand", in: []string{"-v"}, want: true}, + {name: "a positional named version is not a flag", in: []string{"bridge", "add", "version"}, want: false}, + {name: "a false attached value is honored", in: []string{"--version=false"}, want: false}, + {name: "a flag value named like a version flag is not requested", in: []string{"--config", "-v", "bridge", "list"}, want: false}, + {name: "a flag value named like version is not requested", in: []string{"--endpoint", "--version", "bridge", "list"}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := versionRequested(tt.in, testClients); got != tt.want { + t.Errorf("versionRequested(%v) = %v, want %v", tt.in, got, tt.want) + } + }) + } +} + +func TestWarnDeprecated(t *testing.T) { + var out strings.Builder + warnDeprecated(&out, []string{"endpoint"}) + got := out.String() + if !strings.Contains(got, "-endpoint") || !strings.Contains(got, "--endpoint") || !strings.Contains(got, "deprecated") { + t.Errorf("warning = %q, want the old and new forms and the word deprecated", got) + } +} diff --git a/internal/cli/serve.go b/internal/cli/serve.go new file mode 100644 index 0000000..5c5fd8d --- /dev/null +++ b/internal/cli/serve.go @@ -0,0 +1,128 @@ +package cli + +import ( + "fmt" + "log/slog" + "os" + "os/signal" + "syscall" + + "github.com/spf13/cobra" + "github.com/tailscale/aperture-cli/internal/bridges" + "github.com/tailscale/aperture-cli/internal/config" +) + +// serveCommand builds the serve command: one Machine and one Route on a +// fixed loopback port until a signal stops it (ADR 0008). Serve is a +// foreground process; the operator's supervisor owns restart. +func serveCommand() *cobra.Command { + cmd := &cobra.Command{ + Use: "serve", + Short: "Run a persistent bridge on a fixed port", + Example: ` aperture serve --bridge work + aperture serve --bridge work --port 8992 + aperture serve --bridge work --endpoint e1`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + g, err := loadGlobal(cmd) + if err != nil { + return err + } + bridgeName := flagOrEnv(cmd, "bridge", "APERTURE_BRIDGE") + if bridgeName == "" { + return fmt.Errorf("serve needs --bridge, the name of a configured bridge") + } + // bridgeByName never creates: serve runs a bridge that exists, + // and a typo must not register a device. + b, err := bridgeByName(g, bridgeName) + if err != nil { + return err + } + port, err := servePort(flagInt(cmd, "port"), b) + if err != nil { + return err + } + remote, err := serveEndpoint(g, b, flagOrEnv(cmd, "endpoint", "APERTURE_ENDPOINT")) + if err != nil { + return err + } + + ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + defer stop() + + machines := bridges.NewMachines(g.Debug) + defer func() { + if err := machines.Close(); err != nil { + slog.Error("shutting down bridges", "err", err) + } + }() + emit := eventPrinter(cmd, g.Debug) + mc, err := machines.For(b) + if err != nil { + return err + } + // A signal during bring-up cancels the context. That is a clean + // stop, not a failure: a supervisor restarting on non-zero would + // flap a serve the operator asked to stop. + if err := mc.Open(ctx, emit); err != nil { + if ctx.Err() != nil { + return nil + } + return err + } + route, err := mc.RouteTo(ctx, remote, port, emit) + if err != nil { + if ctx.Err() != nil { + return nil + } + return err + } + fmt.Fprintf(cmd.ErrOrStderr(), "serving %s through %s on %s\n", remote, b.Name, route.LocalURL) + <-ctx.Done() + fmt.Fprintln(cmd.ErrOrStderr(), "shutting down") + return nil + }, + } + cmd.Flags().IntP("port", "p", 0, "loopback port to bind, over the bridge's stored port") + return cmd +} + +// flagInt reads an integer flag. Zero means unset: a port of zero is never +// a real choice, so --port=0 behaves as if the flag was not passed. +func flagInt(cmd *cobra.Command, name string) int { + value, _ := cmd.Flags().GetInt(name) + return value +} + +// servePort decides the port serve binds: the flag, then the bridge's +// stored port. Neither is an error, because a random port is exactly what +// serve exists to avoid (ADR 0008). +func servePort(flagPort int, b config.Bridge) (int, error) { + if flagPort != 0 { + if flagPort < 0 || flagPort > 65535 { + return 0, fmt.Errorf("port %d is outside 1-65535", flagPort) + } + return flagPort, nil + } + if b.Port != 0 { + return b.Port, nil + } + return 0, fmt.Errorf("bridge %s has no port; pass --port or set one with bridge add", b.Name) +} + +// serveEndpoint decides the URL serve fronts. A name resolves to the saved +// endpoint's URL; an endpoint that goes through another bridge is a +// contradiction, because serve's Route runs on this bridge's Machine. +func serveEndpoint(g *config.Global, b config.Bridge, value string) (string, error) { + if value == "" { + return config.DefaultLocation, nil + } + if named, ok := g.Settings.EndpointNamed(value); ok { + if bridged, isBridged := named.(config.BridgeEndpoint); isBridged && bridged.BridgeID() != b.ID { + return "", fmt.Errorf("endpoint %q goes through another bridge", value) + } + return named.URL(), nil + } + return config.ParseEndpointURL(value) +} diff --git a/internal/cli/serve_test.go b/internal/cli/serve_test.go new file mode 100644 index 0000000..25c2d43 --- /dev/null +++ b/internal/cli/serve_test.go @@ -0,0 +1,82 @@ +package cli + +import ( + "testing" + + "github.com/tailscale/aperture-cli/internal/config" +) + +func TestServePort(t *testing.T) { + stored := config.Bridge{Name: "work", Port: 8991} + tests := []struct { + name string + flagPort int + bridge config.Bridge + want int + wantErr bool + }{ + {name: "flag wins over the stored port", flagPort: 8992, bridge: stored, want: 8992}, + {name: "stored port without the flag", bridge: stored, want: 8991}, + {name: "neither is an error", bridge: config.Bridge{Name: "work"}, wantErr: true}, + {name: "negative flag", flagPort: -1, bridge: stored, wantErr: true}, + {name: "too large flag", flagPort: 65536, bridge: stored, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := servePort(tt.flagPort, tt.bridge) + if tt.wantErr { + if err == nil { + t.Fatalf("servePort(%d, %+v) = %d, want an error", tt.flagPort, tt.bridge, got) + } + return + } + if err != nil { + t.Fatalf("servePort: %v", err) + } + if got != tt.want { + t.Errorf("servePort(%d, %+v) = %d, want %d", tt.flagPort, tt.bridge, got, tt.want) + } + }) + } +} + +func TestServeEndpoint(t *testing.T) { + g := &config.Global{Settings: config.Settings{ + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://direct"), Name: "e1"}, + {Endpoint: config.Bridged("http://bridged", "bridge-abc123"), Name: "e2"}, + {Endpoint: config.Bridged("http://other-bridge", "bridge-fedcba"), Name: "e3"}, + }, + }} + serve := config.Bridge{ID: "bridge-abc123", Name: "work"} + tests := []struct { + name string + value string + want string + wantErr bool + }{ + {name: "default is the well-known location", value: "", want: config.DefaultLocation}, + {name: "direct name", value: "e1", want: "http://direct"}, + {name: "name through this bridge", value: "e2", want: "http://bridged"}, + {name: "name through another bridge is a contradiction", value: "e3", wantErr: true}, + {name: "raw URL", value: "http://aperture.example.com", want: "http://aperture.example.com"}, + {name: "bare host", value: "ai", want: "http://ai"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := serveEndpoint(g, serve, tt.value) + if tt.wantErr { + if err == nil { + t.Fatalf("serveEndpoint(%q) = %q, want an error", tt.value, got) + } + return + } + if err != nil { + t.Fatalf("serveEndpoint(%q): %v", tt.value, err) + } + if got != tt.want { + t.Errorf("serveEndpoint(%q) = %q, want %q", tt.value, got, tt.want) + } + }) + } +} diff --git a/internal/clients/claudecode/claudecode.go b/internal/clients/claudecode/claudecode.go index 88c8a50..1fbb226 100644 --- a/internal/clients/claudecode/claudecode.go +++ b/internal/clients/claudecode/claudecode.go @@ -7,6 +7,7 @@ package claudecode import ( + "fmt" "maps" "os" "os/exec" @@ -166,17 +167,116 @@ func (c *Client) modelStep(g *config.Global, p config.ProviderInfo, b backend) m }} } +// Spec implements clients.Client. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, b, m, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + return c.buildSpec(g, p, b, m, args) +} + +// resolve picks the provider, backend and model for a headless launch: the +// flags, then LastLaunch when it still fits, then the first compatible +// provider with its first backend and default model. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, backend, string, error) { + last := g.LastLaunch + replaying := last.LastClientName == name + if provider == "" && replaying { + if p, ok := g.Provider(last.LastProviderID); ok && len(backendsFor(p)) > 0 { + provider = p.ID + } + } + if provider == "" { + provs := compatibleProviders(g.Providers) + if len(provs) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no provider on this endpoint supports Claude Code") + } + provider = provs[0].ID + } + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no provider %q; this endpoint offers %s", provider, listOrNone(providerIDs(compatibleProviders(g.Providers)))) + } + choices := dedupedBackendsFor(p) + if len(choices) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("provider %q does not support Claude Code; it offers %s", provider, listOrNone(endpointsOf(p))) + } + b := choices[0] + sameProvider := replaying && p.ID == last.LastProviderID + if sameProvider { + all := backendsFor(p) + if i := slices.IndexFunc(all, func(x backend) bool { return x.id == last.LastBackendType }); i >= 0 { + b = all[i] + } + } + + if !b.picksModel { + if model != "" { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("%s picks its own models for Claude Code; drop --model", b.displayName) + } + return p, b, "", nil + } + models := fqnModels(p) + if model == "" && sameProvider && slices.Contains(models, last.LastModel) { + model = last.LastModel + } + if model == "" && len(models) > 0 { + model = models[0] + } + return p, b, model, nil +} + +// providerIDs renders provider ids for an error message. +func providerIDs(provs []config.ProviderInfo) []string { + ids := make([]string, 0, len(provs)) + for _, p := range provs { + ids = append(ids, p.ID) + } + return ids +} + +// endpointsOf returns the endpoints a provider advertises, sorted. +func endpointsOf(p config.ProviderInfo) []string { + return slices.Sorted(maps.Keys(p.SupportedEndpoints)) +} + +// listOrNone joins items for an error message, or says none when empty. +func listOrNone(items []string) string { + if len(items) == 0 { + return "none" + } + return strings.Join(items, ", ") +} + +// launch wraps buildSpec in the menu flow: the exec runs as a tea.Cmd and the +// menu pops when the child exits. func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, model string) menu.Result { - if err := checkClaudeSettings(); err != nil { + spec, err := c.buildSpec(g, p, b, model, nil) + if err != nil { return errorResult(err.Error()) } + launchFn := c.launchFn + if launchFn == nil { + launchFn = clients.Launch + } + return menu.Result{Cmd: launchFn(spec), PopOnDone: true} +} + +// buildSpec assembles the launch for one provider, backend and model and +// records the launch state. extra passes through to the binary verbatim, +// after every flag aperture sets, for headless invocations. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, b backend, model string, extra []string) (clients.LaunchSpec, error) { + if err := checkClaudeSettings(); err != nil { + return clients.LaunchSpec{}, err + } bin := clients.FindBinary(binaryName, c.CommonPaths()) if bin == "" { bin = binaryName } env, err := envForBackend(g.ApertureHost, b) if err != nil { - return errorResult(err.Error()) + return clients.LaunchSpec{}, err } // Bedrock derives per-tier model env vars from the provider's model list. maps.Copy(env, tierModelEnv(b, p)) @@ -188,6 +288,7 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, mode if g.Settings.YoloMode { args = append(args, "--dangerously-skip-permissions") } + args = append(args, extra...) _ = g.RecordLaunch(config.LaunchState{ LastClientName: name, @@ -196,17 +297,12 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, mode LastModel: model, }) - launchFn := c.launchFn - if launchFn == nil { - launchFn = clients.Launch - } - cmd := launchFn(clients.LaunchSpec{ + return clients.LaunchSpec{ Binary: bin, Args: args, Env: env, Debug: g.Debug, - }) - return menu.Result{Cmd: cmd, PopOnDone: true} + }, nil } // Replay implements clients.Client. diff --git a/internal/clients/claudecode/claudecode_test.go b/internal/clients/claudecode/claudecode_test.go index add03d4..6ddf791 100644 --- a/internal/clients/claudecode/claudecode_test.go +++ b/internal/clients/claudecode/claudecode_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "slices" + "strings" "testing" tea "github.com/charmbracelet/bubbletea" @@ -385,3 +386,107 @@ func lower(s string) string { } return string(b) } + +func TestSpec(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + anthropic := config.ProviderInfo{ID: "anthropic", Models: []string{"claude-opus", "claude-sonnet"}, SupportedEndpoints: map[string]bool{config.EndpointAnthropicMessages: true}} + bedrock := config.ProviderInfo{ID: "bedrock", Models: []string{"us.anthropic.claude-opus-4"}, SupportedEndpoints: map[string]bool{config.EndpointBedrockInvoke: true}} + openai := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5"}, SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true}} + newGlobal := func() *config.Global { + return &config.Global{ApertureHost: testHost, Providers: []config.ProviderInfo{anthropic, bedrock, openai}} + } + c := &Client{} + + // Defaults: the first compatible provider, its first backend, its first model. + spec, err := c.Spec(newGlobal(), "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["ANTHROPIC_BASE_URL"] != testHost || spec.Env["ANTHROPIC_MODEL"] != "claude-opus" { + t.Errorf("default env = %v, want the Anthropic backend on its first model", spec.Env) + } + + // Flags win, and passthrough args land after aperture's own. + g := newGlobal() + g.Settings.YoloMode = true + spec, err = c.Spec(g, "anthropic", "anthropic/claude-sonnet", []string{"--search"}) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["ANTHROPIC_MODEL"] != "claude-sonnet" { + t.Errorf("flag env = %v, want the flag model", spec.Env) + } + if want := []string{"--dangerously-skip-permissions", "--search"}; !slices.Equal(spec.Args, want) { + t.Errorf("args = %v, want %v", spec.Args, want) + } + + // A backend that picks its own models rejects --model and sets none. + spec, err = c.Spec(newGlobal(), "bedrock", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["CLAUDE_CODE_USE_BEDROCK"] != "1" || spec.Env["ANTHROPIC_DEFAULT_OPUS_MODEL"] != "us.anthropic.claude-opus-4" { + t.Errorf("bedrock env = %v, want Bedrock routing with tier models", spec.Env) + } + if _, ok := spec.Env["ANTHROPIC_MODEL"]; ok { + t.Errorf("bedrock env sets ANTHROPIC_MODEL: %v", spec.Env) + } + if _, err := c.Spec(newGlobal(), "bedrock", "bedrock/x", nil); err == nil || !strings.Contains(err.Error(), "drop --model") { + t.Errorf("bedrock with --model: %v, want the flag rejected", err) + } + + // LastLaunch fills empty flags when it still fits. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "anthropic", LastBackendType: "anthropic", LastModel: "anthropic/claude-sonnet"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["ANTHROPIC_MODEL"] != "claude-sonnet" { + t.Errorf("replay env = %v, want LastLaunch's model", spec.Env) + } + + // LastLaunch picks the backend again when the provider still offers it: + // anthropic also takes /v1/messages for z.ai, so zai is a real choice. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "anthropic", LastBackendType: "zai"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["ANTHROPIC_DEFAULT_HAIKU_MODEL"] != "glm-5-turbo" { + t.Errorf("replay backend env = %v, want the z.ai backend", spec.Env) + } + + // A stale LastBackendType falls back to the default backend. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "anthropic", LastBackendType: "vertex"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if _, ok := spec.Env["ANTHROPIC_DEFAULT_HAIKU_MODEL"]; ok || spec.Env["ANTHROPIC_AUTH_TOKEN"] != "-" { + t.Errorf("stale backend env = %v, want the default Anthropic backend", spec.Env) + } + + // Errors: an unknown provider names what the endpoint offers, an + // incompatible one names what it does offer. + if _, err := c.Spec(newGlobal(), "ghost", "", nil); err == nil || !strings.Contains(err.Error(), "anthropic, bedrock") { + t.Errorf("unknown provider: %v, want the compatible ids in the error", err) + } + if _, err := c.Spec(newGlobal(), "openai", "", nil); err == nil || !strings.Contains(err.Error(), "/v1/responses") { + t.Errorf("incompatible provider: %v, want the offered endpoint named", err) + } + + // A conflicting ~/.claude/settings.json blocks the headless launch too. + claudeDir := filepath.Join(os.Getenv("HOME"), ".claude") + if err := os.MkdirAll(claudeDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(claudeDir, "settings.json"), []byte(`{"env":{"ANTHROPIC_BASE_URL":"x"}}`), 0o644); err != nil { + t.Fatal(err) + } + if _, err := c.Spec(newGlobal(), "", "", nil); err == nil || !strings.Contains(err.Error(), "ANTHROPIC_BASE_URL") { + t.Errorf("conflicting settings: %v, want the conflict named", err) + } +} diff --git a/internal/clients/codex/codex.go b/internal/clients/codex/codex.go index ff2afdb..6760209 100644 --- a/internal/clients/codex/codex.go +++ b/internal/clients/codex/codex.go @@ -6,10 +6,12 @@ package codex import ( "fmt" + "log/slog" "os" "os/exec" "runtime" "slices" + "strings" tea "github.com/charmbracelet/bubbletea" "github.com/tailscale/aperture-cli/internal/clients" @@ -149,15 +151,94 @@ func (c *Client) modelStep(g *config.Global, p config.ProviderInfo) menu.Result }} } +// Spec implements clients.Client. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, m, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + return c.buildSpec(g, p, m, args), nil +} + +// resolve picks the provider and model for a headless launch: the flags, +// then LastLaunch when it still fits, then the first compatible provider +// and its default model. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, string, error) { + if provider != "" { + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, "", fmt.Errorf("no provider %q; the endpoint offers %s", provider, providerIDs(compatibleProviders(g.Providers))) + } + if !p.SupportsEndpoint(config.EndpointOpenAIResponses) { + return config.ProviderInfo{}, "", fmt.Errorf("provider %q does not support OpenAI /v1/responses", provider) + } + if model == "" { + model = defaultModel(p) + } + return p, model, nil + } + if last := g.LastLaunch; last.LastClientName == name { + if p, ok := g.Provider(last.LastProviderID); ok && p.SupportsEndpoint(config.EndpointOpenAIResponses) { + if model == "" { + if slices.Contains(fqnModels(p), last.LastModel) { + model = last.LastModel + } else { + model = defaultModel(p) + } + } + return p, model, nil + } + } + provs := compatibleProviders(g.Providers) + if len(provs) == 0 { + return config.ProviderInfo{}, "", fmt.Errorf("no providers support OpenAI /v1/responses") + } + if model == "" { + model = defaultModel(provs[0]) + } + return provs[0], model, nil +} + +// defaultModel returns the provider's first model, the same pick the menu +// makes when it offers exactly one. +func defaultModel(p config.ProviderInfo) string { + if models := fqnModels(p); len(models) > 0 { + return models[0] + } + return "" +} + +// providerIDs renders provider ids for an error message. +func providerIDs(provs []config.ProviderInfo) string { + if len(provs) == 0 { + return "none" + } + ids := make([]string, 0, len(provs)) + for _, p := range provs { + ids = append(ids, p.ID) + } + return strings.Join(ids, ", ") +} + // launch builds the exec spec, records the launch state, and returns a tea.Cmd. func (c *Client) launch(g *config.Global, p config.ProviderInfo, model string) menu.Result { + return menu.Result{Cmd: clients.Launch(c.buildSpec(g, p, model, nil)), PopOnDone: true} +} + +// buildSpec assembles the launch for one provider and model and records the +// launch state. extra passes through to the binary verbatim, after every +// flag aperture sets, for headless invocations. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, model string, extra []string) clients.LaunchSpec { bin := clients.FindBinary(binaryName, c.CommonPaths()) if bin == "" { bin = binaryName } modelCatalogPath, cleanup, err := prepareModelCatalog(bin, g.Providers, p.ID) - if err != nil && g.Debug { - fmt.Fprintf(os.Stderr, "\r\n[debug] unable to prepare Codex model aliases: %v\r\n", err) + if err != nil { + slog.Warn("unable to prepare Codex model aliases", "err", err) + if g.Debug { + fmt.Fprintf(os.Stderr, "\r\n[debug] unable to prepare Codex model aliases: %v\r\n", err) + } } args, env := apertureLaunchConfig(g.ApertureHost, modelCatalogPath) if model != "" { @@ -166,6 +247,7 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, model string) m if g.Settings.YoloMode { args = append(args, "--dangerously-bypass-approvals-and-sandbox") } + args = append(args, extra...) _ = g.RecordLaunch(config.LaunchState{ LastClientName: name, @@ -174,14 +256,13 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, model string) m LastModel: model, }) - cmd := clients.Launch(clients.LaunchSpec{ + return clients.LaunchSpec{ Binary: bin, Args: args, Env: env, Cleanup: cleanup, Debug: g.Debug, - }) - return menu.Result{Cmd: cmd, PopOnDone: true} + } } // Replay implements clients.Client. diff --git a/internal/clients/codex/codex_test.go b/internal/clients/codex/codex_test.go index c79412f..ff7f00c 100644 --- a/internal/clients/codex/codex_test.go +++ b/internal/clients/codex/codex_test.go @@ -216,3 +216,83 @@ func TestReplay_StaleProvider(t *testing.T) { t.Error("Replay with missing binary should return nil") } } + +// The headless launch (ADR 0007): flags win, LastLaunch fills what they +// leave empty, and anything left goes to the first compatible provider. +func TestSpec(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + responses := map[string]bool{config.EndpointOpenAIResponses: true} + openai := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5", "gpt-5-mini"}, SupportedEndpoints: responses} + anthropic := config.ProviderInfo{ID: "anthropic", Models: []string{"claude"}, SupportedEndpoints: map[string]bool{config.EndpointAnthropicMessages: true}} + newGlobal := func() *config.Global { + return &config.Global{ApertureHost: testHost, Providers: []config.ProviderInfo{anthropic, openai}} + } + c := &Client{} + argsContain := func(args []string, want ...string) bool { + for i := 0; i+len(want) <= len(args); i++ { + if slices.Equal(args[i:i+len(want)], want) { + return true + } + } + return false + } + + // Defaults: the only compatible provider, its first model. + spec, err := c.Spec(newGlobal(), "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if !argsContain(spec.Args, "--model", "openai/gpt-5") { + t.Errorf("default args = %v, want the first model of the compatible provider", spec.Args) + } + + // An explicit provider with no model falls back to that provider's + // default model, same as the unflagged path. + spec, err = c.Spec(newGlobal(), "openai", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if !argsContain(spec.Args, "--model", "openai/gpt-5") { + t.Errorf("explicit provider args = %v, want the provider's default model", spec.Args) + } + + // Flags win, and passthrough args land after aperture's own. + spec, err = c.Spec(newGlobal(), "openai", "openai/gpt-5-mini", []string{"--search"}) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if !argsContain(spec.Args, "--model", "openai/gpt-5-mini") || spec.Args[len(spec.Args)-1] != "--search" { + t.Errorf("flag args = %v, want the flag model and the passthrough last", spec.Args) + } + + // LastLaunch fills empty flags when it still fits. + g := newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "openai", LastModel: "openai/gpt-5-mini"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if !argsContain(spec.Args, "--model", "openai/gpt-5-mini") { + t.Errorf("replay args = %v, want LastLaunch's model", spec.Args) + } + + // A stale model in LastLaunch falls back to the provider's default model. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "openai", LastModel: "openai/stale-model"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec with stale model: %v", err) + } + if !argsContain(spec.Args, "--model", "openai/gpt-5") { + t.Errorf("stale model args = %v, want the provider's default model", spec.Args) + } + + // Errors: an unknown provider names what the endpoint offers, an + // incompatible one says why. + if _, err := c.Spec(newGlobal(), "ghost", "", nil); err == nil || !strings.Contains(err.Error(), "openai") { + t.Errorf("unknown provider: %v, want the compatible ids in the error", err) + } + if _, err := c.Spec(newGlobal(), "anthropic", "", nil); err == nil || !strings.Contains(err.Error(), "responses") { + t.Errorf("incompatible provider: %v, want the missing API named", err) + } +} diff --git a/internal/clients/copilot/copilot.go b/internal/clients/copilot/copilot.go index e3a4dc0..bc0d2b3 100644 --- a/internal/clients/copilot/copilot.go +++ b/internal/clients/copilot/copilot.go @@ -4,6 +4,7 @@ package copilot import ( + "fmt" "os/exec" "slices" "strings" @@ -147,7 +148,92 @@ func (c *Client) modelStep(g *config.Global, p config.ProviderInfo, b backend) m }} } +// Spec implements clients.Client. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, b, m, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + return c.buildSpec(g, p, b, m, args), nil +} + +// resolve picks the provider, backend and model for a headless launch: the +// flags, then LastLaunch when it still fits, then the first compatible +// provider with its first backend and default model. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, backend, string, error) { + if provider != "" { + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no provider %q; the endpoint offers %s", provider, providerIDs(compatibleProviders(g.Providers))) + } + bs := backendsFor(p) + if len(bs) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("provider %q supports none of the endpoints %s speaks: %s", provider, name, backendNames()) + } + if model == "" { + if models := fqnModels(p); len(models) > 0 { + model = models[0] + } + } + return p, bs[0], model, nil + } + if last := g.LastLaunch; last.LastClientName == name { + if p, ok := g.Provider(last.LastProviderID); ok { + bs := backendsFor(p) + if i := slices.IndexFunc(bs, func(b backend) bool { return b.id == last.LastBackendType }); i >= 0 { + if model == "" { + if slices.Contains(fqnModels(p), last.LastModel) { + model = last.LastModel + } else if models := fqnModels(p); len(models) > 0 { + model = models[0] + } + } + return p, bs[i], model, nil + } + } + } + provs := compatibleProviders(g.Providers) + if len(provs) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no providers support %s; it speaks %s", name, backendNames()) + } + p := provs[0] + if model == "" { + if models := fqnModels(p); len(models) > 0 { + model = models[0] + } + } + return p, backendsFor(p)[0], model, nil +} + +// providerIDs renders provider ids for an error message. +func providerIDs(provs []config.ProviderInfo) string { + if len(provs) == 0 { + return "none" + } + ids := make([]string, 0, len(provs)) + for _, p := range provs { + ids = append(ids, p.ID) + } + return strings.Join(ids, ", ") +} + +// backendNames renders the backends Copilot speaks for an error message. +func backendNames() string { + names := make([]string, 0, len(backends)) + for _, b := range backends { + names = append(names, b.displayName) + } + return strings.Join(names, ", ") +} + func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, model string) menu.Result { + return menu.Result{Cmd: clients.Launch(c.buildSpec(g, p, b, model, nil)), PopOnDone: true} +} + +// buildSpec assembles the launch for one provider, backend and model and +// records the launch state. extra passes through to the binary verbatim, +// for headless invocations. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, b backend, model string, extra []string) clients.LaunchSpec { bin := clients.FindBinary(binaryName, c.CommonPaths()) if bin == "" { bin = binaryName @@ -162,12 +248,12 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, mode LastModel: model, }) - cmd := clients.Launch(clients.LaunchSpec{ + return clients.LaunchSpec{ Binary: bin, + Args: extra, Env: env, Debug: g.Debug, - }) - return menu.Result{Cmd: cmd, PopOnDone: true} + } } func buildEnv(apertureHost string, b backend, model string) map[string]string { diff --git a/internal/clients/copilot/copilot_test.go b/internal/clients/copilot/copilot_test.go index 42a03cf..5ca581c 100644 --- a/internal/clients/copilot/copilot_test.go +++ b/internal/clients/copilot/copilot_test.go @@ -1,6 +1,8 @@ package copilot import ( + "slices" + "strings" "testing" "github.com/tailscale/aperture-cli/internal/config" @@ -127,3 +129,90 @@ func TestStripProviderPrefix(t *testing.T) { } } } + +// The headless launch (ADR 0007): flags win, LastLaunch fills what they +// leave empty, and anything left goes to the first compatible provider on +// its first backend. +func TestSpec(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + both := map[string]bool{config.EndpointOpenAIChat: true, config.EndpointOpenAIResponses: true} + bedrock := config.ProviderInfo{ID: "bedrock", Models: []string{"claude"}, SupportedEndpoints: map[string]bool{config.EndpointBedrockConverse: true}} + openai := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5", "gpt-5-mini"}, SupportedEndpoints: both} + anthropic := config.ProviderInfo{ID: "anthropic", Models: []string{"claude"}, SupportedEndpoints: map[string]bool{config.EndpointAnthropicMessages: true}} + newGlobal := func() *config.Global { + return &config.Global{ApertureHost: testHost, Providers: []config.ProviderInfo{bedrock, openai, anthropic}} + } + c := &Client{} + + // Defaults: the first compatible provider, its first backend, its first model. + spec, err := c.Spec(newGlobal(), "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["COPILOT_PROVIDER_TYPE"] != "openai" || spec.Env["COPILOT_PROVIDER_WIRE_API"] != "completions" || spec.Env["COPILOT_MODEL"] != "gpt-5" { + t.Errorf("default env = %v, want openai chat completions on gpt-5", spec.Env) + } + if len(spec.Args) != 0 { + t.Errorf("default args = %v, want none", spec.Args) + } + + // An explicit provider with no model falls back to that provider's + // default model, same as the unflagged path. + spec, err = c.Spec(newGlobal(), "openai", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["COPILOT_MODEL"] != "gpt-5" { + t.Errorf("explicit provider env = %v, want the provider's default model", spec.Env) + } + + // Flags win, and passthrough args come through verbatim. + spec, err = c.Spec(newGlobal(), "anthropic", "anthropic/claude", []string{"--allow-all"}) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["COPILOT_PROVIDER_TYPE"] != "anthropic" || spec.Env["COPILOT_MODEL"] != "claude" { + t.Errorf("flag env = %v, want the flagged provider and model", spec.Env) + } + if !slices.Equal(spec.Args, []string{"--allow-all"}) { + t.Errorf("flag args = %v, want the passthrough", spec.Args) + } + + // LastLaunch fills empty flags, backend included, and an explicit model still wins. + g := newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastBackendType: "openai_responses", LastProviderID: "openai", LastModel: "openai/gpt-5-mini"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["COPILOT_PROVIDER_WIRE_API"] != "responses" || spec.Env["COPILOT_MODEL"] != "gpt-5-mini" { + t.Errorf("replay env = %v, want the LastLaunch backend and model", spec.Env) + } + spec, err = c.Spec(g, "", "openai/gpt-5", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["COPILOT_PROVIDER_WIRE_API"] != "responses" || spec.Env["COPILOT_MODEL"] != "gpt-5" { + t.Errorf("model flag env = %v, want the flag model on the LastLaunch backend", spec.Env) + } + + // A stale model in LastLaunch falls back to the provider's default model. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastBackendType: "openai_responses", LastProviderID: "openai", LastModel: "openai/stale-model"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec with stale model: %v", err) + } + if spec.Env["COPILOT_MODEL"] != "gpt-5" { + t.Errorf("stale model env = %v, want the provider's default model", spec.Env) + } + + // Errors: an unknown provider names the compatible ids, an incompatible + // one names the backends it lacks. + if _, err := c.Spec(newGlobal(), "ghost", "", nil); err == nil || !strings.Contains(err.Error(), "openai, anthropic") { + t.Errorf("unknown provider: %v, want the compatible ids in the error", err) + } + if _, err := c.Spec(newGlobal(), "bedrock", "", nil); err == nil || !strings.Contains(err.Error(), "OpenAI Chat Completions") { + t.Errorf("incompatible provider: %v, want the Copilot backends named", err) + } +} diff --git a/internal/clients/gemini/gemini.go b/internal/clients/gemini/gemini.go index d7ddfb3..89badc1 100644 --- a/internal/clients/gemini/gemini.go +++ b/internal/clients/gemini/gemini.go @@ -137,14 +137,88 @@ func (c *Client) backendStep(g *config.Global, p config.ProviderInfo) menu.Resul }} } +// Spec implements clients.Client. Gemini has no model menu, so model is +// passed to the CLI as --model when set and left to the CLI otherwise. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, b, m, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + return c.buildSpec(g, p, b, m, args) +} + +// resolve picks the provider, backend and model for a headless launch: the +// flags, then LastLaunch when it still fits, then the first compatible +// provider and its first backend. An empty model leaves the choice to the CLI. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, backend, string, error) { + last := g.LastLaunch + if last.LastClientName != name { + last = config.LaunchState{} + } + if provider == "" { + if p, ok := g.Provider(last.LastProviderID); ok && len(backendsFor(p)) > 0 { + provider = p.ID + } else if provs := compatibleProviders(g.Providers); len(provs) > 0 { + provider = provs[0].ID + } else { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no provider supports Gemini CLI (Vertex or Gemini API); the endpoint offers %s", providerIDs(g.Providers)) + } + } + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no provider %q; the endpoint offers %s", provider, providerIDs(compatibleProviders(g.Providers))) + } + bs := backendsFor(p) + if len(bs) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("provider %q does not serve Vertex Gemini or the Gemini API", provider) + } + // Only the provider that LastLaunch recorded carries its backend and model over. + if p.ID != last.LastProviderID { + return p, bs[0], model, nil + } + if model == "" { + model = last.LastModel + } + b := bs[0] + if i := slices.IndexFunc(bs, func(candidate backend) bool { return candidate.id == last.LastBackendType }); i >= 0 { + b = bs[i] + } + return p, b, model, nil +} + +// providerIDs renders provider ids for an error message. +func providerIDs(provs []config.ProviderInfo) string { + if len(provs) == 0 { + return "none" + } + ids := make([]string, 0, len(provs)) + for _, p := range provs { + ids = append(ids, p.ID) + } + return strings.Join(ids, ", ") +} + +// launch builds the menu's launch for one provider and backend. The menu +// never picks a model, so the CLI default applies. func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend) menu.Result { + spec, err := c.buildSpec(g, p, b, "", nil) + if err != nil { + return errorResult(err.Error()) + } + return menu.Result{Cmd: clients.Launch(spec), PopOnDone: true} +} + +// buildSpec assembles the launch for one provider, backend and model and +// records the launch state. extra passes through to the binary verbatim, +// after every flag aperture sets, for headless invocations. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, b backend, model string, extra []string) (clients.LaunchSpec, error) { // Gemini CLI 0.40+ refuses custom base URLs that aren't https:// with a // fully-qualified domain name (it allows http only for literal // localhost / 127.0.0.1). Aperture's default "http://ai" short hostname - // won't work — block the launch with a clear message rather than let + // won't work, so block the launch with a clear message rather than let // Gemini fail with an authentication error after start. if err := validateHost(g.ApertureHost); err != nil { - return errorResult(err.Error()) + return clients.LaunchSpec{}, err } bin := clients.FindBinary(binaryName, c.CommonPaths()) if bin == "" { @@ -152,7 +226,7 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend) menu } geminiHome, err := writeConfig(b.authType) if err != nil { - return errorResult("Failed to write Gemini config: " + err.Error()) + return clients.LaunchSpec{}, fmt.Errorf("Failed to write Gemini config: %w", err) } host := strings.TrimRight(g.ApertureHost, "/") @@ -177,20 +251,24 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend) menu if g.Settings.YoloMode { args = append(args, "--yolo") } + if model != "" { + args = append(args, "--model", model) + } + args = append(args, extra...) _ = g.RecordLaunch(config.LaunchState{ LastClientName: name, LastBackendType: b.id, LastProviderID: p.ID, + LastModel: model, }) - cmd := clients.Launch(clients.LaunchSpec{ + return clients.LaunchSpec{ Binary: bin, Args: args, Env: env, Debug: g.Debug, - }) - return menu.Result{Cmd: cmd, PopOnDone: true} + }, nil } // Replay implements clients.Client. diff --git a/internal/clients/gemini/gemini_test.go b/internal/clients/gemini/gemini_test.go index 6edb045..495bcb1 100644 --- a/internal/clients/gemini/gemini_test.go +++ b/internal/clients/gemini/gemini_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "os" "path/filepath" + "slices" + "strings" "testing" "github.com/tailscale/aperture-cli/internal/config" @@ -99,3 +101,79 @@ func TestWriteConfig_Vertex(t *testing.T) { t.Errorf("selectedType = %q, want vertex-ai", s.Security.Auth.SelectedType) } } + +// The headless launch (ADR 0007): flags win, LastLaunch fills what they +// leave empty, and anything left goes to the first compatible provider. +func TestSpec(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + openai := config.ProviderInfo{ID: "openai", SupportedEndpoints: map[string]bool{config.EndpointOpenAIChat: true}} + vertexOnly := config.ProviderInfo{ID: "vertex", SupportedEndpoints: map[string]bool{config.EndpointVertexGemini: true}} + both := config.ProviderInfo{ID: "both", SupportedEndpoints: map[string]bool{ + config.EndpointVertexGemini: true, + config.EndpointGemini: true, + }} + newGlobal := func() *config.Global { + return &config.Global{ApertureHost: "https://ai.example.com", Providers: []config.ProviderInfo{openai, vertexOnly, both}} + } + c := &Client{} + argsContain := func(args []string, want ...string) bool { + for i := range len(args) - len(want) + 1 { + if slices.Equal(args[i:i+len(want)], want) { + return true + } + } + return false + } + + // Defaults: the first compatible provider, its first backend, and no + // --model so the CLI picks its own. + spec, err := c.Spec(newGlobal(), "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["GOOGLE_VERTEX_BASE_URL"] == "" || len(spec.Args) != 0 { + t.Errorf("default env %v args %v, want the vertex backend and no flags", spec.Env, spec.Args) + } + + // Flags win, and passthrough args land after aperture's own. + spec, err = c.Spec(newGlobal(), "both", "gemini-2.5-pro", []string{"--checkpointing"}) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if !argsContain(spec.Args, "--model", "gemini-2.5-pro") || spec.Args[len(spec.Args)-1] != "--checkpointing" { + t.Errorf("flag args = %v, want the flag model and the passthrough last", spec.Args) + } + + // LastLaunch fills empty flags, including the backend it recorded. + g := newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "both", LastBackendType: "gemini", LastModel: "gemini-2.5-flash"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["GEMINI_BASE_URL"] == "" || !argsContain(spec.Args, "--model", "gemini-2.5-flash") { + t.Errorf("replay env %v args %v, want LastLaunch's backend and model", spec.Env, spec.Args) + } + + // LastLaunch from another client, or naming a provider that is gone, + // is ignored. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: "OpenAI Codex", LastProviderID: "both", LastBackendType: "gemini", LastModel: "gpt-5"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if spec.Env["GOOGLE_VERTEX_BASE_URL"] == "" || len(spec.Args) != 0 { + t.Errorf("other client's state leaked: env %v args %v", spec.Env, spec.Args) + } + + // Errors: an unknown provider names the compatible ids, an + // incompatible one says which APIs it lacks. + if _, err := c.Spec(newGlobal(), "ghost", "", nil); err == nil || !strings.Contains(err.Error(), "vertex, both") { + t.Errorf("unknown provider: %v, want the compatible ids in the error", err) + } + if _, err := c.Spec(newGlobal(), "openai", "", nil); err == nil || !strings.Contains(err.Error(), "Vertex Gemini or the Gemini API") { + t.Errorf("incompatible provider: %v, want the missing APIs named", err) + } +} diff --git a/internal/clients/hermes/hermes.go b/internal/clients/hermes/hermes.go index 0d7b840..549f1b8 100644 --- a/internal/clients/hermes/hermes.go +++ b/internal/clients/hermes/hermes.go @@ -5,6 +5,7 @@ package hermes import ( + "fmt" "os/exec" "slices" "strings" @@ -105,18 +106,92 @@ func (c *Client) modelStep(g *config.Global, p config.ProviderInfo) menu.Result return menu.Result{Next: &menu.Menu{Title: "Choose a default model for " + name + " via " + p.DisplayName() + ":", Items: items}} } +// Spec implements clients.Client. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, m, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + return c.buildSpec(g, p, m, args), nil +} + +// resolve picks the provider and model for a headless launch: the flags, +// then LastLaunch when it still fits, then the first compatible provider +// and its default model. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, string, error) { + compatible := compatibleProviders(g.Providers) + if provider != "" { + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, "", fmt.Errorf("no provider %q; compatible providers are %s", provider, providerIDs(compatible)) + } + if !providerMatches(p) { + return config.ProviderInfo{}, "", fmt.Errorf("provider %q does not support OpenAI /v1/chat/completions; compatible providers are %s", provider, providerIDs(compatible)) + } + if model == "" { + model = defaultModel(p) + } + return p, model, nil + } + if last := g.LastLaunch; last.LastClientName == name && last.LastBackendType == backendType { + if p, ok := g.Provider(last.LastProviderID); ok && providerMatches(p) { + if model == "" { + if slices.Contains(fqnModels(p), last.LastModel) { + model = last.LastModel + } else { + model = defaultModel(p) + } + } + return p, model, nil + } + } + if len(compatible) == 0 { + return config.ProviderInfo{}, "", fmt.Errorf("no providers support OpenAI /v1/chat/completions") + } + if model == "" { + model = defaultModel(compatible[0]) + } + return compatible[0], model, nil +} + +// defaultModel returns the provider's first model in provider_id/model_id +// form, or empty when it lists none. +func defaultModel(p config.ProviderInfo) string { + if models := fqnModels(p); len(models) > 0 { + return models[0] + } + return "" +} + +// providerIDs renders provider ids for an error message. +func providerIDs(provs []config.ProviderInfo) string { + if len(provs) == 0 { + return "none" + } + ids := make([]string, 0, len(provs)) + for _, p := range provs { + ids = append(ids, p.ID) + } + return strings.Join(ids, ", ") +} + func (c *Client) launch(g *config.Global, p config.ProviderInfo, model string) menu.Result { + return menu.Result{Cmd: clients.Launch(c.buildSpec(g, p, model, nil)), PopOnDone: true} +} + +// buildSpec assembles the launch for one provider and model and records the +// launch state. extra passes through to the binary verbatim, after every +// flag aperture sets, for headless invocations. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, model string, extra []string) clients.LaunchSpec { bin := clients.FindBinary(binaryName, c.CommonPaths()) if bin == "" { bin = binaryName } - env := buildEnv(g.ApertureHost, model) - args := buildArgs(g.Settings.YoloMode) + args := append(buildArgs(g.Settings.YoloMode), extra...) _ = g.RecordLaunch(config.LaunchState{ LastClientName: name, LastBackendType: backendType, LastProviderID: p.ID, LastModel: model, }) - cmd := clients.Launch(clients.LaunchSpec{Binary: bin, Args: args, Env: env, Debug: g.Debug}) - return menu.Result{Cmd: cmd, PopOnDone: true} + return clients.LaunchSpec{Binary: bin, Args: args, Env: buildEnv(g.ApertureHost, model), Debug: g.Debug} } func buildEnv(apertureHost, model string) map[string]string { diff --git a/internal/clients/hermes/hermes_test.go b/internal/clients/hermes/hermes_test.go index 60d5d70..2b7efbc 100644 --- a/internal/clients/hermes/hermes_test.go +++ b/internal/clients/hermes/hermes_test.go @@ -2,6 +2,7 @@ package hermes import ( "slices" + "strings" "testing" "github.com/tailscale/aperture-cli/internal/config" @@ -102,3 +103,82 @@ func TestInstallUninstall(t *testing.T) { t.Errorf("Uninstall = %+v", got) } } + +func TestSpec(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + chat := map[string]bool{config.EndpointOpenAIChat: true} + openrouter := config.ProviderInfo{ID: "openrouter", Models: []string{"gpt-5", "gpt-5-mini"}, SupportedEndpoints: chat} + responses := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5"}, SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true}} + anthropic := config.ProviderInfo{ID: "anthropic", Models: []string{"claude"}, SupportedEndpoints: map[string]bool{config.EndpointAnthropicMessages: true}} + newGlobal := func() *config.Global { + return &config.Global{ApertureHost: testHost, Providers: []config.ProviderInfo{anthropic, responses, openrouter}} + } + c := &Client{} + baseArgs := []string{"--provider", "custom"} + + // Defaults: the first compatible provider and its first model. + spec, err := c.Spec(newGlobal(), "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if got := spec.Env["HERMES_INFERENCE_MODEL"]; got != "gpt-5" { + t.Errorf("default model = %q, want the first compatible provider's first model", got) + } + if !slices.Equal(spec.Args, baseArgs) { + t.Errorf("default args = %v, want %v", spec.Args, baseArgs) + } + + // Flags win, and passthrough args land after aperture's own. + g := newGlobal() + g.Settings.YoloMode = true + spec, err = c.Spec(g, "openrouter", "openrouter/gpt-5-mini", []string{"--resume", "abc"}) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if got := spec.Env["HERMES_INFERENCE_MODEL"]; got != "gpt-5-mini" { + t.Errorf("flag model = %q, want gpt-5-mini", got) + } + if want := []string{"--provider", "custom", "--yolo", "--resume", "abc"}; !slices.Equal(spec.Args, want) { + t.Errorf("flag args = %v, want %v", spec.Args, want) + } + + // An explicit provider with no model takes that provider's first model. + spec, err = c.Spec(newGlobal(), "openrouter", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if got := spec.Env["HERMES_INFERENCE_MODEL"]; got != "gpt-5" { + t.Errorf("provider default model = %q, want the provider's first model", got) + } + + // LastLaunch fills empty flags when it still fits. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastBackendType: backendType, LastProviderID: "openrouter", LastModel: "openrouter/gpt-5-mini"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if got := spec.Env["HERMES_INFERENCE_MODEL"]; got != "gpt-5-mini" { + t.Errorf("replay model = %q, want LastLaunch's model", got) + } + + // A stale model in LastLaunch falls back to the provider's default model. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastBackendType: backendType, LastProviderID: "openrouter", LastModel: "openrouter/stale-model"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec with stale model: %v", err) + } + if got := spec.Env["HERMES_INFERENCE_MODEL"]; got != "gpt-5" { + t.Errorf("stale model = %q, want the provider's first model", got) + } + + // Errors: an unknown provider names the compatible ids, an incompatible + // one says which API it lacks. + if _, err := c.Spec(newGlobal(), "ghost", "", nil); err == nil || !strings.Contains(err.Error(), "openrouter") { + t.Errorf("unknown provider: %v, want the compatible ids in the error", err) + } + if _, err := c.Spec(newGlobal(), "anthropic", "", nil); err == nil || !strings.Contains(err.Error(), "/v1/chat/completions") { + t.Errorf("incompatible provider: %v, want the missing API named", err) + } +} diff --git a/internal/clients/launch.go b/internal/clients/launch.go index 5a5f47d..581ee3a 100644 --- a/internal/clients/launch.go +++ b/internal/clients/launch.go @@ -27,13 +27,13 @@ type LaunchSpec struct { Debug bool } -// Launch returns a tea.Cmd that runs the given spec via tea.ExecProcess and -// emits menu.ExecDoneMsg when the child exits. If spec.Binary is empty, -// the command returns immediately with an error. -func Launch(spec LaunchSpec) tea.Cmd { +// Command builds the child process for the spec: Env overlaid on the +// current environment, the standard streams attached, and the resolved Env +// and Args dumped to stderr when Debug is set. The TUI hands it to +// tea.ExecProcess; a headless launch runs it directly. +func (spec LaunchSpec) Command() (*exec.Cmd, error) { if spec.Binary == "" { - err := fmt.Errorf("binary path is empty") - return func() tea.Msg { return menu.ExecDoneMsg{Err: err} } + return nil, fmt.Errorf("binary path is empty") } envPairs := os.Environ() @@ -56,7 +56,17 @@ func Launch(spec LaunchSpec) tea.Cmd { cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + return cmd, nil +} +// Launch returns a tea.Cmd that runs the given spec via tea.ExecProcess and +// emits menu.ExecDoneMsg when the child exits. If spec.Binary is empty, +// the command returns immediately with an error. +func Launch(spec LaunchSpec) tea.Cmd { + cmd, err := spec.Command() + if err != nil { + return func() tea.Msg { return menu.ExecDoneMsg{Err: err} } + } return tea.ExecProcess(cmd, func(err error) tea.Msg { if spec.Cleanup != nil { spec.Cleanup() diff --git a/internal/clients/opencode/opencode.go b/internal/clients/opencode/opencode.go index 3f952bd..f2cd382 100644 --- a/internal/clients/opencode/opencode.go +++ b/internal/clients/opencode/opencode.go @@ -7,9 +7,12 @@ package opencode import ( + "errors" + "fmt" "os" "os/exec" "path/filepath" + "strings" tea "github.com/charmbracelet/bubbletea" "github.com/tailscale/aperture-cli/internal/clients" @@ -114,13 +117,24 @@ func (c *Client) providerStep(g *config.Global) menu.Result { } func (c *Client) launch(g *config.Global, p config.ProviderInfo) menu.Result { + spec, err := c.buildSpec(g, p, "", nil) + if err != nil { + return errorResult("Failed to write OpenCode config: " + err.Error()) + } + return menu.Result{Cmd: clients.Launch(spec), PopOnDone: true} +} + +// buildSpec writes the provider config and records the launch state. model +// becomes OpenCode's --model and extra passes through verbatim, both for +// headless invocations; the menu passes neither. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, model string, extra []string) (clients.LaunchSpec, error) { bin := clients.FindBinary(binaryName, c.CommonPaths()) if bin == "" { bin = binaryName } configPath, cleanup, err := writeProviderConfig(g.ApertureHost, p) if err != nil { - return errorResult("Failed to write OpenCode config: " + err.Error()) + return clients.LaunchSpec{}, err } env := map[string]string{ @@ -133,22 +147,86 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo) menu.Result { env["AWS_REGION"] = "us-east-1" } - // OpenCode has no documented yolo flag today; keep Args empty. The - // provider config written above conveys available models; the user - // picks one inside OpenCode itself. + // OpenCode has no documented yolo flag today. The provider config written + // above conveys available models; model is OpenCode's own --model, given + // only by a headless invocation. + var args []string + if model != "" { + args = append(args, "--model", model) + } + args = append(args, extra...) + _ = g.RecordLaunch(config.LaunchState{ LastClientName: name, LastBackendType: "openai", // historical; OpenCode's abstract backend LastProviderID: p.ID, + LastModel: model, }) - cmd := clients.Launch(clients.LaunchSpec{ + return clients.LaunchSpec{ Binary: bin, + Args: args, Env: env, Cleanup: cleanup, Debug: g.Debug, - }) - return menu.Result{Cmd: cmd, PopOnDone: true} + }, nil +} + +// Spec implements clients.Client. OpenCode picks its model in its own UI, +// so model only ever comes from a headless flag or replay: a --model flag +// passes through verbatim, in the provider/model form OpenCode's own +// --model takes. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, model, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + spec, err := c.buildSpec(g, p, model, args) + if err != nil { + return clients.LaunchSpec{}, fmt.Errorf("write OpenCode config: %w", err) + } + return spec, nil +} + +// resolve picks the provider, and the model when the flag left it empty, +// for a headless launch: the flags, then LastLaunch when it still fits, +// then the first compatible provider and no model. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, string, error) { + if provider != "" { + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, "", fmt.Errorf("no provider %q; the endpoint offers %s", provider, providerIDs(compatibleProviders(g.Providers))) + } + if !providerMatches(p) { + return config.ProviderInfo{}, "", fmt.Errorf("provider %q advertises no endpoint OpenCode can use; the endpoint offers %s", provider, providerIDs(compatibleProviders(g.Providers))) + } + return p, model, nil + } + if last := g.LastLaunch; last.LastClientName == name { + if p, ok := g.Provider(last.LastProviderID); ok && providerMatches(p) { + if model == "" { + model = last.LastModel + } + return p, model, nil + } + } + provs := compatibleProviders(g.Providers) + if len(provs) == 0 { + return config.ProviderInfo{}, "", errors.New("no providers support an OpenCode protocol") + } + return provs[0], model, nil +} + +// providerIDs renders provider ids for an error message. +func providerIDs(provs []config.ProviderInfo) string { + if len(provs) == 0 { + return "none" + } + ids := make([]string, 0, len(provs)) + for _, p := range provs { + ids = append(ids, p.ID) + } + return strings.Join(ids, ", ") } // Replay implements clients.Client. diff --git a/internal/clients/opencode/opencode_test.go b/internal/clients/opencode/opencode_test.go index 794a157..1e67cf2 100644 --- a/internal/clients/opencode/opencode_test.go +++ b/internal/clients/opencode/opencode_test.go @@ -3,8 +3,12 @@ package opencode import ( "encoding/json" "os" + "os/exec" + "os/signal" "path/filepath" "slices" + "strings" + "syscall" "testing" "github.com/tailscale/aperture-cli/internal/config" @@ -218,3 +222,178 @@ func TestWriteProviderConfig(t *testing.T) { }) } } + +// The headless launch (ADR 0007): flags win, LastLaunch supplies the +// provider, and OpenCode picks its own model unless --model says otherwise. +func TestSpec(t *testing.T) { + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) + + embed := config.ProviderInfo{ID: "embed", Models: []string{"embed-1"}, SupportedEndpoints: map[string]bool{"/v1/embeddings": true}} + openai := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5", "gpt-5-mini"}, SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true}} + anthropic := config.ProviderInfo{ID: "anthropic", Models: []string{"claude"}, SupportedEndpoints: map[string]bool{config.EndpointAnthropicMessages: true}} + newGlobal := func() *config.Global { + return &config.Global{ApertureHost: testHost, Providers: []config.ProviderInfo{embed, openai, anthropic}} + } + c := &Client{} + + // Defaults: the first compatible provider and no model, because OpenCode + // picks its model in its own UI. The incompatible provider ahead of it + // is skipped. + g := newGlobal() + spec, err := c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if len(spec.Args) != 0 { + t.Errorf("default args = %q, want none", spec.Args) + } + if g.LastLaunch.LastProviderID != "openai" { + t.Errorf("provider = %q, want the first compatible one", g.LastLaunch.LastProviderID) + } + if spec.Env["OPENCODE_CONFIG"] == "" { + t.Errorf("default env = %v, want OPENCODE_CONFIG set", spec.Env) + } + + // Flags win, a --model passes through verbatim, and passthrough args + // land after aperture's own. + g = newGlobal() + spec, err = c.Spec(g, "anthropic", "anthropic/anthropic/claude", []string{"--session", "s1"}) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if want := []string{"--model", "anthropic/anthropic/claude", "--session", "s1"}; !slices.Equal(spec.Args, want) { + t.Errorf("flag args = %q, want %q", spec.Args, want) + } + if g.LastLaunch.LastProviderID != "anthropic" { + t.Errorf("provider = %q, want the flag's provider", g.LastLaunch.LastProviderID) + } + + // LastLaunch supplies the provider while it still fits, and no model + // when none was ever recorded: the menu records none and headless + // replay invents none. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "openai"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if len(spec.Args) != 0 { + t.Errorf("replay args = %q, want none", spec.Args) + } + + // A model an earlier headless launch recorded replays when the flags + // supply neither provider nor model. + g = newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: "openai", LastModel: "gpt-5-mini"} + spec, err = c.Spec(g, "", "", nil) + if err != nil { + t.Fatalf("Spec: %v", err) + } + if want := []string{"--model", "gpt-5-mini"}; !slices.Equal(spec.Args, want) { + t.Errorf("replayed model args = %q, want %q", spec.Args, want) + } + + // LastLaunch for a provider that is gone or no longer compatible is + // ignored in favor of the default. + for _, stale := range []string{"ghost", "embed"} { + g := newGlobal() + g.LastLaunch = config.LaunchState{LastClientName: name, LastProviderID: stale} + if _, err := c.Spec(g, "", "", nil); err != nil { + t.Fatalf("Spec with stale %q: %v", stale, err) + } + if g.LastLaunch.LastProviderID != "openai" { + t.Errorf("stale %q provider = %q, want the default", stale, g.LastLaunch.LastProviderID) + } + } + + // Errors: an unknown provider names the compatible ids, an + // incompatible one says why, and no compatible provider says so. + if _, err := c.Spec(newGlobal(), "ghost", "", nil); err == nil || !strings.Contains(err.Error(), "openai, anthropic") { + t.Errorf("unknown provider: %v, want the compatible ids in the error", err) + } + if _, err := c.Spec(newGlobal(), "embed", "", nil); err == nil || !strings.Contains(err.Error(), "OpenCode can use") { + t.Errorf("incompatible provider: %v, want the reason in the error", err) + } + none := &config.Global{ApertureHost: testHost, Providers: []config.ProviderInfo{embed}} + if _, err := c.Spec(none, "", "", nil); err == nil || !strings.Contains(err.Error(), "OpenCode protocol") { + t.Errorf("no compatible provider: %v, want the protocol named", err) + } + // The menu launch names no model and no extra args. + spec, err = c.buildSpec(newGlobal(), openai, "", nil) + if err != nil { + t.Fatalf("buildSpec: %v", err) + } + if len(spec.Args) != 0 { + t.Errorf("menu args = %q, want none", spec.Args) + } +} + +// Two concurrent launches must not share a per-launch config file: the +// second write would replace the first, and the first child to exit would +// delete it under the second. +func TestWriteProviderConfigIsUniquePerLaunch(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + provs := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5"}, SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true}} + + first, cleanupFirst, err := writeProviderConfig(testHost, provs) + if err != nil { + t.Fatalf("first writeProviderConfig: %v", err) + } + second, cleanupSecond, err := writeProviderConfig(testHost, provs) + if err != nil { + cleanupFirst() + t.Fatalf("second writeProviderConfig: %v", err) + } + defer cleanupSecond() + cleanupFirst() + if first == second { + t.Fatalf("both launches wrote %q", first) + } + if _, err := os.Stat(second); err != nil { + t.Errorf("the second launch's config is gone: %v", err) + } +} + +// A write that fails partway through (disk full, quota) must not leave the +// partially written temp file behind in ~/.opencode. +func TestWriteProviderConfigRemovesTempFileOnWriteFailure(t *testing.T) { + if os.Getenv("TEST_OPENCODE_WRITE_FAILURE") == "1" { + signal.Ignore(syscall.SIGXFSZ) + defer signal.Reset(syscall.SIGXFSZ) + var orig syscall.Rlimit + if err := syscall.Getrlimit(syscall.RLIMIT_FSIZE, &orig); err != nil { + t.Fatal(err) + } + defer syscall.Setrlimit(syscall.RLIMIT_FSIZE, &orig) + limited := syscall.Rlimit{Cur: 1, Max: orig.Max} + if err := syscall.Setrlimit(syscall.RLIMIT_FSIZE, &limited); err != nil { + t.Fatal(err) + } + + provs := config.ProviderInfo{ID: "openai", Models: []string{"gpt-5"}, SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true}} + if _, _, err := writeProviderConfig(testHost, provs); err == nil { + t.Fatal("writeProviderConfig: want a write failure under the file-size limit") + } + return + } + + home := t.TempDir() + cmd := exec.Command(os.Args[0], "-test.run=^TestWriteProviderConfigRemovesTempFileOnWriteFailure$") + cmd.Env = append(os.Environ(), "TEST_OPENCODE_WRITE_FAILURE=1", "HOME="+home) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("child process failed: %v\n%s", err, out) + } + + entries, err := os.ReadDir(filepath.Join(home, ".opencode")) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), "tmp_aperture_config_") { + t.Errorf("leaked temp file %s after a write failure", e.Name()) + } + } +} diff --git a/internal/clients/opencode/sdk.go b/internal/clients/opencode/sdk.go index 2716610..7e23066 100644 --- a/internal/clients/opencode/sdk.go +++ b/internal/clients/opencode/sdk.go @@ -72,10 +72,12 @@ func pickSDK(p config.ProviderInfo, apertureHost string) (npm string, options ma return "", nil } -// writeProviderConfig writes the per-launch OpenCode config under -// ~/.opencode/tmp_aperture_config.json and returns the path plus a cleanup -// function that removes the file. The config defines one provider (the -// chosen one) mapped to the SDK picked from its advertised endpoints. +// writeProviderConfig writes the per-launch OpenCode config under ~/.opencode +// as a uniquely named file, and returns the path plus a cleanup function that +// removes it. The name is per launch: a fixed name would have two concurrent +// launches overwrite each other's config, and the first child to exit would +// unlink the second one's. The config defines one provider (the chosen one) +// mapped to the SDK picked from its advertised endpoints. func writeProviderConfig(apertureHost string, p config.ProviderInfo) (string, func(), error) { npm, options := pickSDK(p, apertureHost) @@ -113,9 +115,20 @@ func writeProviderConfig(apertureHost string, p config.ProviderInfo) (string, fu if err := os.MkdirAll(configDir, 0o700); err != nil { return "", nil, err } - path := filepath.Join(configDir, "tmp_aperture_config.json") - if err := os.WriteFile(path, data, 0o600); err != nil { + f, err := os.CreateTemp(configDir, "tmp_aperture_config_*.json") + if err != nil { + return "", nil, err + } + path := f.Name() + remove := func() { os.Remove(path) } + if _, err := f.Write(data); err != nil { + f.Close() + remove() + return "", nil, err + } + if err := f.Close(); err != nil { + remove() return "", nil, err } - return path, func() { os.Remove(path) }, nil + return path, remove, nil } diff --git a/internal/clients/pilike/extension.go b/internal/clients/pilike/extension.go index a0165f7..f6eeae1 100644 --- a/internal/clients/pilike/extension.go +++ b/internal/clients/pilike/extension.go @@ -1,6 +1,8 @@ package pilike import ( + "time" + "encoding/json" "os" "path/filepath" @@ -149,16 +151,27 @@ const extensionGlob = "tmp_aperture_provider_*.js" // afterwards does not affect that process. Removal errors are ignored for the // same reason they are unlikely to matter — a file another process holds open // is not a reason to fail this launch. +// +// Files younger than sweepGrace are left alone: a sibling launch may still be +// waiting for its child to read the extension, and the gap between the write +// and the read is exactly where a concurrent sweep would strike. func sweepOrphanedExtensions(dir string) { matches, err := filepath.Glob(filepath.Join(dir, extensionGlob)) if err != nil { return } for _, m := range matches { + if info, err := os.Stat(m); err == nil && time.Since(info.ModTime()) < sweepGrace { + continue + } _ = os.Remove(m) } } +// sweepGrace is the age a stranded extension must reach before a later +// launch sweeps it. It covers a sibling launch's startup window. +const sweepGrace = time.Minute + // writeProviderExtension writes the per-launch extension and returns its // path plus a cleanup function that removes the file. Extensions stranded by // an earlier launch are swept first; see sweepOrphanedExtensions. diff --git a/internal/clients/pilike/pilike.go b/internal/clients/pilike/pilike.go index 4b7e059..fa3d9f9 100644 --- a/internal/clients/pilike/pilike.go +++ b/internal/clients/pilike/pilike.go @@ -16,6 +16,7 @@ package pilike import ( + "fmt" "os/exec" "slices" "strings" @@ -214,7 +215,90 @@ func (c *Client) modelStep(g *config.Global, p config.ProviderInfo, b backend) m }} } +// Spec implements clients.Client. +func (c *Client) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + p, b, m, err := c.resolve(g, provider, model) + if err != nil { + return clients.LaunchSpec{}, err + } + return c.buildSpec(g, p, b, m, args) +} + +// resolve picks the provider, backend, and model for a headless launch: the +// flags, then LastLaunch when it still fits, then the first compatible +// provider with its default backend and model. +func (c *Client) resolve(g *config.Global, provider, model string) (config.ProviderInfo, backend, string, error) { + if provider != "" { + p, ok := g.Provider(provider) + if !ok { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no provider %q; the endpoint offers %s", provider, providerIDs(compatibleProviders(g.Providers))) + } + return launchTarget(p, "", model) + } + if last := g.LastLaunch; last.LastClientName == c.V.Name { + if p, ok := g.Provider(last.LastProviderID); ok && isCompatible(p) { + if model == "" && slices.Contains(fqnModels(p), last.LastModel) { + model = last.LastModel + } + return launchTarget(p, last.LastBackendType, model) + } + } + provs := compatibleProviders(g.Providers) + if len(provs) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("no providers support %s", c.V.Name) + } + return launchTarget(provs[0], "", model) +} + +// launchTarget checks that p can serve this client, then picks its backend +// and model. preferredID is kept when p still serves it, and an empty model +// becomes p's first model. +func launchTarget(p config.ProviderInfo, preferredID, model string) (config.ProviderInfo, backend, string, error) { + if len(p.Models) == 0 { + return config.ProviderInfo{}, backend{}, "", fmt.Errorf("provider %q lists no models", p.ID) + } + b, err := launchBackend(p, preferredID) + if err != nil { + return config.ProviderInfo{}, backend{}, "", err + } + if model == "" { + model = fqnModels(p)[0] + } + return p, b, model, nil +} + +// launchBackend returns the backend preferredID names when p still serves +// it, and otherwise the first backend p serves, the most preferred. It +// errors when p serves none. +func launchBackend(p config.ProviderInfo, preferredID string) (backend, error) { + if b, ok := backendByID(preferredID); ok && providerSupports(p, b) { + return b, nil + } + bs := backendsFor(p) + if len(bs) == 0 { + names := make([]string, len(backends)) + for i, b := range backends { + names[i] = b.displayName + } + return backend{}, fmt.Errorf("provider %q serves no protocol this client can use; it needs one of: %s", p.ID, strings.Join(names, ", ")) + } + return bs[0], nil +} + +// launch builds the menu's foreground launch and records the launch state. func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, m string) menu.Result { + spec, err := c.buildSpec(g, p, b, m, nil) + if err != nil { + return errorResult("Failed to " + err.Error()) + } + return menu.Result{Cmd: clients.Launch(spec), PopOnDone: true} +} + +// buildSpec writes the per-launch extension, assembles the launch for one +// provider, backend, and model, and records the launch state. extra passes +// through to the binary verbatim, after every flag aperture sets, for +// headless invocations. +func (c *Client) buildSpec(g *config.Global, p config.ProviderInfo, b backend, m string, extra []string) (clients.LaunchSpec, error) { bin := clients.FindBinary(c.V.BinaryName, c.CommonPaths()) if bin == "" { bin = c.V.BinaryName @@ -222,10 +306,9 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, m st extPath, cleanup, err := writeProviderExtension(c.V, g.ApertureHost, p, b) if err != nil { - return errorResult("Failed to write " + c.V.Name + " provider extension: " + err.Error()) + return clients.LaunchSpec{}, fmt.Errorf("write %s provider extension: %w", c.V.Name, err) } - - args := buildArgs(c.V, extPath, p.ID, m, g.Settings.YoloMode) + args := append(buildArgs(c.V, extPath, p.ID, m, g.Settings.YoloMode), extra...) _ = g.RecordLaunch(config.LaunchState{ LastClientName: c.V.Name, @@ -234,13 +317,12 @@ func (c *Client) launch(g *config.Global, p config.ProviderInfo, b backend, m st LastModel: m, }) - cmd := clients.Launch(clients.LaunchSpec{ + return clients.LaunchSpec{ Binary: bin, Args: args, Cleanup: cleanup, Debug: g.Debug, - }) - return menu.Result{Cmd: cmd, PopOnDone: true} + }, nil } // buildArgs assembles the harness's command line: load the generated @@ -327,16 +409,34 @@ func (c *Client) QuickSelectLabel(g *config.Global) string { return label } +// isCompatible reports whether p can serve this client: it lists a model +// and advertises a protocol at least one backend speaks. +func isCompatible(p config.ProviderInfo) bool { + return len(p.Models) > 0 && len(backendsFor(p)) > 0 +} + func compatibleProviders(all []config.ProviderInfo) []config.ProviderInfo { var out []config.ProviderInfo for _, p := range all { - if len(p.Models) > 0 && len(backendsFor(p)) > 0 { + if isCompatible(p) { out = append(out, p) } } return out } +// providerIDs renders the provider IDs for an error message. +func providerIDs(provs []config.ProviderInfo) string { + if len(provs) == 0 { + return "none" + } + ids := make([]string, len(provs)) + for i, p := range provs { + ids[i] = p.ID + } + return strings.Join(ids, ", ") +} + func backendsFor(p config.ProviderInfo) []backend { var out []backend for _, b := range backends { diff --git a/internal/clients/pilike/pilike_test.go b/internal/clients/pilike/pilike_test.go index 7ff6d16..3ac3e47 100644 --- a/internal/clients/pilike/pilike_test.go +++ b/internal/clients/pilike/pilike_test.go @@ -7,6 +7,7 @@ import ( "slices" "strings" "testing" + "time" "github.com/tailscale/aperture-cli/internal/config" ) @@ -859,6 +860,12 @@ func TestWriteProviderExtension_SweepsOrphans(t *testing.T) { if err := os.WriteFile(path, []byte("// orphan\n"), 0o600); err != nil { t.Fatal(err) } + // Orphans only count as stranded once they pass the sweep's grace + // window: a sibling launch's fresh extension is not one. + orphaned := time.Now().Add(-2 * time.Minute) + if err := os.Chtimes(path, orphaned, orphaned); err != nil { + t.Fatal(err) + } stale = append(stale, path) } // A file that is not ours must survive the sweep. @@ -891,3 +898,169 @@ func TestWriteProviderExtension_SweepsOrphans(t *testing.T) { t.Errorf("extensions in the config dir = %v, want only %v", matches, []string{path}) } } + +// flagValue returns the argument after flag, or "" when flag is absent. +func flagValue(args []string, flag string) string { + i := slices.Index(args, flag) + if i < 0 || i+1 >= len(args) { + return "" + } + return args[i+1] +} + +// TestSpec covers the headless launch: flags win, LastLaunch fills what the +// flags leave empty, the backend follows LastBackendType when the provider +// still serves it, and passthrough args come last. +func TestSpec(t *testing.T) { + isolateConfigDir(t) + bedrock := config.ProviderInfo{ID: "bedrock", Models: []string{"m"}, SupportedEndpoints: map[string]bool{config.EndpointBedrockConverse: true}} + empty := config.ProviderInfo{ID: "empty", SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true}} + openai := config.ProviderInfo{ + ID: "openai", + Models: []string{"gpt-5", "gpt-5-mini"}, + SupportedEndpoints: map[string]bool{config.EndpointOpenAIResponses: true, config.EndpointOpenAIChat: true}, + } + anthropic := config.ProviderInfo{ID: "anthropic", Models: []string{"claude"}, SupportedEndpoints: map[string]bool{config.EndpointAnthropicMessages: true}} + providers := []config.ProviderInfo{bedrock, empty, openai, anthropic} + global := func(last config.LaunchState) *config.Global { + return &config.Global{ApertureHost: testHost, Providers: providers, LastLaunch: last} + } + openaiReplay := func(backendID, model string) config.LaunchState { + return config.LaunchState{LastClientName: noYolo.Name, LastBackendType: backendID, LastProviderID: "openai", LastModel: model} + } + c := &Client{V: noYolo} + + cases := []struct { + name string + last config.LaunchState + provider string + model string + extra []string + wantModel string + wantAPI string + }{ + { + name: "defaults_first_compatible_provider_first_backend_and_model", + wantModel: "aperture-openai/gpt-5", wantAPI: "openai-responses", + }, + { + name: "flags_win_and_passthrough_comes_last", + provider: "anthropic", model: "anthropic/claude", extra: []string{"--search"}, + wantModel: "aperture-anthropic/claude", wantAPI: "anthropic-messages", + }, + { + name: "explicit_provider_without_model_takes_its_first_model", + provider: "openai", + wantModel: "aperture-openai/gpt-5", wantAPI: "openai-responses", + }, + { + name: "explicit_model_is_kept", + provider: "openai", model: "openai/gpt-5-mini", + wantModel: "aperture-openai/gpt-5-mini", wantAPI: "openai-responses", + }, + { + name: "replay_fills_empty_model_and_backend", + last: openaiReplay("openai_chat", "openai/gpt-5-mini"), + wantModel: "aperture-openai/gpt-5-mini", wantAPI: "openai-completions", + }, + { + name: "replay_model_gone_falls_back_to_first_model", + last: openaiReplay("openai_chat", "openai/stale-model"), + wantModel: "aperture-openai/gpt-5", wantAPI: "openai-completions", + }, + { + name: "replay_model_flag_beats_last_model", + last: openaiReplay("openai_chat", "openai/gpt-5-mini"), model: "openai/gpt-5", + wantModel: "aperture-openai/gpt-5", wantAPI: "openai-completions", + }, + { + name: "replay_backend_id_gone_falls_back_to_first_backend", + last: openaiReplay("bedrock_legacy", "openai/gpt-5-mini"), + wantModel: "aperture-openai/gpt-5-mini", wantAPI: "openai-responses", + }, + { + name: "replay_backend_dropped_by_provider_falls_back_to_first_backend", + last: openaiReplay("anthropic", "openai/gpt-5-mini"), + wantModel: "aperture-openai/gpt-5-mini", wantAPI: "openai-responses", + }, + { + name: "replay_provider_gone_falls_through_to_first_compatible", + last: config.LaunchState{LastClientName: noYolo.Name, LastBackendType: "openai_chat", LastProviderID: "gone", LastModel: "gone/x"}, + wantModel: "aperture-openai/gpt-5", wantAPI: "openai-responses", + }, + { + name: "another_clients_record_is_ignored", + last: config.LaunchState{LastClientName: "Oh My Pi", LastBackendType: "openai_chat", LastProviderID: "openai", LastModel: "openai/gpt-5-mini"}, + wantModel: "aperture-openai/gpt-5", wantAPI: "openai-responses", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + spec, err := c.Spec(global(tc.last), tc.provider, tc.model, tc.extra) + if err != nil { + t.Fatalf("Spec: %v", err) + } + defer spec.Cleanup() + + if got := flagValue(spec.Args, "--model"); got != tc.wantModel { + t.Errorf("--model = %q, want %q (args %v)", got, tc.wantModel, spec.Args) + } + src, err := os.ReadFile(flagValue(spec.Args, "-e")) + if err != nil { + t.Fatalf("extension unreadable: %v", err) + } + if want := `"api": "` + tc.wantAPI + `"`; !strings.Contains(string(src), want) { + t.Errorf("extension lacks %s:\n%s", want, src) + } + if tail := spec.Args[len(spec.Args)-len(tc.extra):]; !slices.Equal(tail, tc.extra) { + t.Errorf("args end with %v, want passthrough %v", tail, tc.extra) + } + }) + } + + // Errors name what the endpoint does offer, or why the provider cannot + // serve this client. + errCases := []struct { + name string + provider string + want string + }{ + {name: "unknown_provider_names_compatible_ids", provider: "ghost", want: "openai, anthropic"}, + {name: "incompatible_provider_names_needed_protocols", provider: "bedrock", want: "OpenAI Responses"}, + {name: "provider_without_models", provider: "empty", want: "lists no models"}, + } + for _, tc := range errCases { + t.Run(tc.name, func(t *testing.T) { + _, err := c.Spec(global(config.LaunchState{}), tc.provider, "", nil) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("Spec error = %v, want it to contain %q", err, tc.want) + } + }) + } +} + +// A sibling launch's extension is not an orphan: the sweep must leave files +// inside the grace window alone, and take ones old enough to be stranded. +func TestSweepSkipsFreshExtensions(t *testing.T) { + dir := t.TempDir() + fresh := filepath.Join(dir, "tmp_aperture_provider_fresh.js") + if err := os.WriteFile(fresh, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + old := filepath.Join(dir, "tmp_aperture_provider_old.js") + if err := os.WriteFile(old, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + staleTime := time.Now().Add(-2 * time.Minute) + if err := os.Chtimes(old, staleTime, staleTime); err != nil { + t.Fatal(err) + } + + sweepOrphanedExtensions(dir) + if _, err := os.Stat(fresh); err != nil { + t.Errorf("the sweep took a fresh extension: %v", err) + } + if _, err := os.Stat(old); err == nil { + t.Error("the sweep left a stranded extension") + } +} diff --git a/internal/clients/registry.go b/internal/clients/registry.go index 9c546c0..bb58646 100644 --- a/internal/clients/registry.go +++ b/internal/clients/registry.go @@ -42,6 +42,12 @@ type Client interface { // model → launch). Menu(g *config.Global) menu.MenuItem + // Spec builds a headless launch: no menus, no tea. provider and model + // come from command-line flags; empty means LastLaunch when still valid, + // then the first compatible provider and its default model. args pass + // through to the client binary verbatim. + Spec(g *config.Global, provider, model string, args []string) (LaunchSpec, error) + // Replay attempts to re-launch the client using the last-used selection // stored in g.LastLaunch. The TUI only offers replay when the recorded // endpoint is active. Returns nil if the state is stale (binary missing, @@ -93,3 +99,14 @@ func All(g *config.Global) []Client { copy(out, registered) return out } + +// ByName returns the registered client whose binary is name, and whether +// one exists. The command line addresses clients by binary name. +func ByName(name string) (Client, bool) { + for _, c := range registered { + if c.BinaryName() == name { + return c, true + } + } + return nil, false +} diff --git a/internal/config/endpoint.go b/internal/config/endpoint.go index 378805f..3e2f2a1 100644 --- a/internal/config/endpoint.go +++ b/internal/config/endpoint.go @@ -52,6 +52,9 @@ func (e BridgeEndpoint) endpoint() {} type Bridge struct { ID string `json:"id"` Name string `json:"name"` + // Port is the fixed loopback port serve binds for this bridge. Zero means + // no fixed port, and serve asks for --port. + Port int `json:"port,omitempty"` // Tailnet names the tailnet the node last logged in to. Commit records it // after a successful connection, so the connection picker can name the // tailnet before the node starts again. @@ -72,10 +75,12 @@ func ParseEndpointURL(value string) (string, error) { return strings.TrimRight(value, "/"), nil } -// endpointRecord is the settings.json form of an Endpoint. Both kinds share -// one shape, and bridgeId being present tells them apart. The file predates -// the two types and is not changing under existing users. +// endpointRecord is the settings.json form of an EndpointEntry. Both +// endpoint kinds share one shape, and bridgeId being present tells them +// apart. The file predates the two types and is not changing under existing +// users; name is additive and old files carry none. type endpointRecord struct { + Name string `json:"name,omitempty"` URL string `json:"url"` BridgeID string `json:"bridgeId,omitempty"` } @@ -98,13 +103,24 @@ func (r endpointRecord) endpoint() Endpoint { return Direct(r.URL) } -func (e DirectEndpoint) MarshalJSON() ([]byte, error) { return json.Marshal(recordOf(e)) } -func (e BridgeEndpoint) MarshalJSON() ([]byte, error) { return json.Marshal(recordOf(e)) } +// EndpointEntry is one row of the saved endpoint list: an Endpoint and the +// Name a command line addresses it by. The Name stays off the Endpoint +// itself, so endpoint equality stays URL plus bridge. +type EndpointEntry struct { + Endpoint Endpoint + Name string +} + +func (e EndpointEntry) MarshalJSON() ([]byte, error) { + r := recordOf(e.Endpoint) + r.Name = e.Name + return json.Marshal(r) +} -// endpointList holds the Endpoints field of Settings. Its elements are an +// endpointList holds the Endpoints field of Settings. Its elements carry an // interface, and encoding/json cannot decode those without being told the // concrete types. -type endpointList []Endpoint +type endpointList []EndpointEntry func (l *endpointList) UnmarshalJSON(data []byte) error { var records []endpointRecord @@ -113,7 +129,7 @@ func (l *endpointList) UnmarshalJSON(data []byte) error { } *l = make(endpointList, 0, len(records)) for _, r := range records { - *l = append(*l, r.endpoint()) + *l = append(*l, EndpointEntry{Endpoint: r.endpoint(), Name: r.Name}) } return nil } diff --git a/internal/config/endpoint_test.go b/internal/config/endpoint_test.go index 584b9cc..3556640 100644 --- a/internal/config/endpoint_test.go +++ b/internal/config/endpoint_test.go @@ -39,8 +39,8 @@ func TestParseEndpointURL(t *testing.T) { // Two kinds, told apart by the type and not by an empty field, and the same // file shape as before for both. func TestEndpointKindsRoundTripThroughSettings(t *testing.T) { - s := Settings{Endpoints: []Endpoint{Direct("http://ai"), Bridged("http://ai", "bridge-abcdef")}} - data, err := s.Endpoints[1].(BridgeEndpoint).MarshalJSON() + s := Settings{Endpoints: []EndpointEntry{{Endpoint: Direct("http://ai")}, {Endpoint: Bridged("http://ai", "bridge-abcdef")}}} + data, err := s.Endpoints[1].MarshalJSON() if err != nil { t.Fatal(err) } @@ -51,7 +51,7 @@ func TestEndpointKindsRoundTripThroughSettings(t *testing.T) { if err := list.UnmarshalJSON([]byte(`[{"url":"http://ai"},{"url":"http://ai","bridgeId":"bridge-abcdef"}]`)); err != nil { t.Fatal(err) } - if len(list) != 2 || list[0] != Direct("http://ai") || list[1] != Bridged("http://ai", "bridge-abcdef") { + if len(list) != 2 || list[0].Endpoint != Direct("http://ai") || list[1].Endpoint != Bridged("http://ai", "bridge-abcdef") { t.Errorf("decoded = %#v", list) } if Direct("http://ai") == Endpoint(Bridged("http://ai", "")) { diff --git a/internal/config/global.go b/internal/config/global.go index cfa06dd..dd38874 100644 --- a/internal/config/global.go +++ b/internal/config/global.go @@ -3,6 +3,7 @@ package config import ( "fmt" "strings" + "unicode" ) // Global holds the live app state the TUI and every client package share: @@ -43,7 +44,7 @@ func Load() (*Global, error) { } host := DefaultLocation if len(s.Endpoints) > 0 { - host = s.Endpoints[0].URL() + host = s.Endpoints[0].Endpoint.URL() } return &Global{ ApertureHost: host, @@ -65,7 +66,7 @@ func (g *Global) ActiveEndpoint() Endpoint { if len(g.Settings.Endpoints) == 0 { return Direct(DefaultLocation) } - return g.Settings.Endpoints[0] + return g.Settings.Endpoints[0].Endpoint } // SetActiveEndpoint moves ep to the front of the endpoint list, adding it if @@ -73,9 +74,24 @@ func (g *Global) ActiveEndpoint() Endpoint { // of a verified URL edit and goes in the same write; pass nil otherwise. // Bridge activation later rewrites ApertureHost to the local proxy. func (g *Global) SetActiveEndpoint(ep Endpoint, replacing Endpoint) error { - eps := []Endpoint{ep} + head := EndpointEntry{Endpoint: ep} for _, existing := range g.Settings.Endpoints { - if existing != ep && existing != replacing { + if existing.Endpoint == ep { + head = existing + break + } + } + if replacing != nil { + for _, existing := range g.Settings.Endpoints { + if existing.Endpoint == replacing { + head.Name = existing.Name + break + } + } + } + eps := endpointList{head} + for _, existing := range g.Settings.Endpoints { + if existing.Endpoint != ep && existing.Endpoint != replacing { eps = append(eps, existing) } } @@ -99,12 +115,12 @@ func (g *Global) SetApertureHost(url string) error { // there and saves. The active endpoint does not change. func (g *Global) UpsertEndpoint(ep Endpoint) error { for _, existing := range g.Settings.Endpoints { - if existing == ep { + if existing.Endpoint == ep { return nil } } next := g.Settings - next.Endpoints = append(append([]Endpoint(nil), g.Settings.Endpoints...), ep) + next.Endpoints = append(append(endpointList(nil), g.Settings.Endpoints...), EndpointEntry{Endpoint: ep}) if err := SaveSettings(next); err != nil { return err } @@ -115,10 +131,10 @@ func (g *Global) UpsertEndpoint(ep Endpoint) error { // ReplaceEndpoint puts next where old was and saves. The active endpoint // changes only when old was the active one. func (g *Global) ReplaceEndpoint(old, next Endpoint) error { - eps := append([]Endpoint(nil), g.Settings.Endpoints...) + eps := append(endpointList(nil), g.Settings.Endpoints...) oldIdx := -1 for i, existing := range eps { - if existing != old { + if existing.Endpoint != old { continue } oldIdx = i @@ -127,12 +143,13 @@ func (g *Global) ReplaceEndpoint(old, next Endpoint) error { if oldIdx < 0 { return fmt.Errorf("endpoint %s is not configured", old.URL()) } - eps[oldIdx] = next + // A URL edit keeps the row's name: the handle is the user's, not the URL's. + eps[oldIdx] = EndpointEntry{Endpoint: next, Name: eps[oldIdx].Name} deduped := eps[:0] for _, ep := range eps { duplicate := false for _, existing := range deduped { - if existing == ep { + if existing.Endpoint == ep.Endpoint { duplicate = true break } @@ -160,7 +177,7 @@ func (g *Global) removeEndpointAt(idx int) error { if idx < 0 || idx >= len(g.Settings.Endpoints) { return nil } - eps := make([]Endpoint, 0, len(g.Settings.Endpoints)-1) + eps := make(endpointList, 0, len(g.Settings.Endpoints)-1) eps = append(eps, g.Settings.Endpoints[:idx]...) eps = append(eps, g.Settings.Endpoints[idx+1:]...) next := g.Settings @@ -170,7 +187,7 @@ func (g *Global) removeEndpointAt(idx int) error { } g.Settings = next if idx == 0 && len(eps) > 0 { - g.ApertureHost = eps[0].URL() + g.ApertureHost = eps[0].Endpoint.URL() } return nil } @@ -180,7 +197,7 @@ func (g *Global) removeEndpointAt(idx int) error { // not in the list is not an error. func (g *Global) RemoveEndpoint(ep Endpoint) error { for i, existing := range g.Settings.Endpoints { - if i == 0 || existing != ep { + if i == 0 || existing.Endpoint != ep { continue } return g.removeEndpointAt(i) @@ -188,18 +205,41 @@ func (g *Global) RemoveEndpoint(ep Endpoint) error { return nil } +// checkName refuses a name with control characters. Names reach the tabbed +// stdout of bridge list and endpoint list and the status lines scripts +// parse, and a newline or escape would forge rows in them. +func checkName(name string) error { + if strings.ContainsFunc(name, unicode.IsControl) { + return fmt.Errorf("name %q contains control characters", name) + } + return nil +} + // AddBridge creates a bridge named name with a generated ID, saves it and -// returns it. -func (g *Global) AddBridge(name string) (Bridge, error) { +// returns it. port is the fixed port serve binds, zero for none. The name +// is the bridge's command-line handle, so a duplicate is refused without +// case. +func (g *Global) AddBridge(name string, port int) (Bridge, error) { name = strings.TrimSpace(name) if name == "" { return Bridge{}, fmt.Errorf("bridge name is empty") } + if err := checkName(name); err != nil { + return Bridge{}, err + } + if port < 0 || port > 65535 { + return Bridge{}, fmt.Errorf("port %d is outside 0-65535", port) + } + for _, b := range g.Settings.Bridges { + if strings.EqualFold(b.Name, name) { + return Bridge{}, fmt.Errorf("a bridge called %q already exists", name) + } + } id, err := newBridgeID(g.Settings.Bridges) if err != nil { return Bridge{}, err } - p := Bridge{ID: id, Name: name} + p := Bridge{ID: id, Name: name, Port: port} next := g.Settings next.Bridges = append(append([]Bridge(nil), g.Settings.Bridges...), p) if err := SaveSettings(next); err != nil { @@ -232,8 +272,8 @@ func (g *Global) SetBridgeTailnet(id, tailnet string) error { // RemoveBridge deletes bridge id and saves. It refuses while an endpoint // still connects through the bridge. func (g *Global) RemoveBridge(id string) error { - for _, ep := range g.Settings.Endpoints { - if ep, ok := ep.(BridgeEndpoint); ok && ep.BridgeID() == id { + for _, entry := range g.Settings.Endpoints { + if ep, ok := entry.Endpoint.(BridgeEndpoint); ok && ep.BridgeID() == id { return fmt.Errorf("bridge is used by endpoint %s", ep.URL()) } } @@ -253,6 +293,43 @@ func (g *Global) RemoveBridge(id string) error { return nil } +// NameEndpoint saves name as the handle of ep's row in the endpoint list, +// appending ep when it is not listed yet. A name another endpoint holds is +// refused, and so is a name containing ://, because the command line reads +// those as URLs. +func (g *Global) NameEndpoint(ep Endpoint, name string) error { + name = strings.TrimSpace(name) + if name == "" { + return fmt.Errorf("endpoint name is empty") + } + if strings.Contains(name, "://") { + return fmt.Errorf("endpoint name %q looks like a URL", name) + } + if err := checkName(name); err != nil { + return err + } + next := g.Settings + next.Endpoints = append(endpointList(nil), g.Settings.Endpoints...) + found := false + for i, entry := range next.Endpoints { + if entry.Endpoint != ep && entry.Name != "" && strings.EqualFold(entry.Name, name) { + return fmt.Errorf("an endpoint called %q already exists", name) + } + if entry.Endpoint == ep { + next.Endpoints[i].Name = name + found = true + } + } + if !found { + next.Endpoints = append(next.Endpoints, EndpointEntry{Endpoint: ep, Name: name}) + } + if err := SaveSettings(next); err != nil { + return err + } + g.Settings = next + return nil +} + // Bridge returns the configured bridge with id, and whether one exists. func (g *Global) Bridge(id string) (Bridge, bool) { for _, p := range g.Settings.Bridges { diff --git a/internal/config/runlog.go b/internal/config/runlog.go index 2e5b0f4..dc43981 100644 --- a/internal/config/runlog.go +++ b/internal/config/runlog.go @@ -1,6 +1,8 @@ package config import ( + "io" + "log/slog" "os" "path/filepath" ) @@ -42,3 +44,43 @@ func OpenRunLog() (*os.File, error) { } return os.OpenFile(path, flags, 0o600) } + +// runLog is the open run log of this process, nil when the log could not +// open. RaiseRunLog needs the file after StartRunLog chose it. +var runLog *os.File + +// StartRunLog points slog at the run log for the whole process and returns +// the close. 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. +// +// The log is on for every run, not only the interactive one. The run worth +// reading back is the one that went wrong, and nobody knows to pass -debug +// before it does. +func StartRunLog() func() { + f, err := OpenRunLog() + if err != nil { + slog.SetDefault(slog.New(slog.NewTextHandler(io.Discard, nil))) + return func() {} + } + runLog = f + slog.SetDefault(slog.New(slog.NewTextHandler(f, nil))) + return func() { + slog.Info("aperture exiting") + f.Close() + runLog = nil + } +} + +// RaiseRunLog raises the run log to debug level. Call it from the path that +// learned the --debug flag after dispatch; the log itself opens before any +// command runs, so every run logs either way. +func RaiseRunLog() { + if runLog == nil { + return + } + slog.SetDefault(slog.New(slog.NewTextHandler(runLog, &slog.HandlerOptions{Level: slog.LevelDebug}))) +} diff --git a/internal/config/runlog_test.go b/internal/config/runlog_test.go index 5f4636a..f6956e0 100644 --- a/internal/config/runlog_test.go +++ b/internal/config/runlog_test.go @@ -1,6 +1,7 @@ package config_test import ( + "log/slog" "os" "path/filepath" "strings" @@ -55,3 +56,30 @@ func TestOpenRunLogAppendsThenTruncates(t *testing.T) { t.Errorf("log is %d bytes, want the oversized file started over", len(got)) } } + +// Every run logs, whatever path it takes: the log opens before dispatch and +// --debug only raises the level. +func TestStartRunLogAndRaiseRunLog(t *testing.T) { + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) + + closeLog := config.StartRunLog() + slog.Info("at-info") + config.RaiseRunLog() + slog.Debug("at-debug") + closeLog() + + path, err := config.RunLogPath() + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + log := string(data) + if !strings.Contains(log, "at-info") || !strings.Contains(log, "at-debug") { + t.Errorf("run log = %q, want both the info and the debug record", log) + } +} diff --git a/internal/config/settings.go b/internal/config/settings.go index e00f5ac..5be2410 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -33,8 +33,23 @@ type Settings struct { YoloMode bool `json:"yoloMode,omitempty"` } +// settingsFile, when non-empty, is the settings.json path for the whole +// process. main sets it once from --config or APERTURE_CONFIG before Load. +var settingsFile string + +// UseSettingsFile points settings reads and writes at path instead of the +// default location. Call it once at startup, before Load. +func UseSettingsFile(path string) { settingsFile = path } + +// SettingsFileOverride returns the settings path --config or APERTURE_CONFIG +// chose, empty when the process reads and writes the default location. +func SettingsFileOverride() string { return settingsFile } + // settingsPath returns the path to the launcher settings JSON file. func settingsPath() (string, error) { + if settingsFile != "" { + return settingsFile, nil + } dir, err := os.UserConfigDir() if err != nil { return "", err @@ -63,7 +78,7 @@ func LoadSettings() (Settings, error) { return Settings{}, fmt.Errorf("parsing settings: %w", err) } if len(s.Endpoints) == 0 { - s.Endpoints = []Endpoint{Direct(DefaultLocation)} + s.Endpoints = endpointList{{Endpoint: Direct(DefaultLocation)}} } return s, nil } @@ -86,8 +101,26 @@ func SaveSettings(s Settings) error { func defaultSettings() Settings { return Settings{ - Endpoints: []Endpoint{Direct(DefaultLocation)}, + Endpoints: endpointList{{Endpoint: Direct(DefaultLocation)}}, + } +} + +// HasEndpoint reports whether the endpoint list holds ep. +func (s Settings) HasEndpoint(ep Endpoint) bool { + return slices.ContainsFunc(s.Endpoints, func(e EndpointEntry) bool { + return e.Endpoint == ep + }) +} + +// EndpointNamed returns the endpoint saved under name, matched without +// case, and whether one exists. +func (s Settings) EndpointNamed(name string) (Endpoint, bool) { + for _, e := range s.Endpoints { + if e.Name != "" && strings.EqualFold(e.Name, name) { + return e.Endpoint, true + } } + return nil, false } // BridgeStateDir returns the tsnet state directory for one slot of a bridge. diff --git a/internal/config/settings_test.go b/internal/config/settings_test.go new file mode 100644 index 0000000..9a274fa --- /dev/null +++ b/internal/config/settings_test.go @@ -0,0 +1,84 @@ +package config + +import ( + "encoding/json" + "path/filepath" + "testing" +) + +// A bridge written before ports existed must load with no port, and a port +// must survive the round trip. +func TestBridgePortRoundTrip(t *testing.T) { + var old Bridge + if err := json.Unmarshal([]byte(`{"id":"bridge-abc123","name":"Work"}`), &old); err != nil { + t.Fatal(err) + } + if old.Port != 0 { + t.Errorf("old record gained a port: %+v", old) + } + data, err := json.Marshal(Bridge{ID: "bridge-abc123", Name: "Work", Port: 8991}) + if err != nil { + t.Fatal(err) + } + if string(data) != `{"id":"bridge-abc123","name":"Work","port":8991}` { + t.Errorf("marshal = %s", data) + } + var back Bridge + if err := json.Unmarshal(data, &back); err != nil { + t.Fatal(err) + } + if back.Port != 8991 { + t.Errorf("port = %d after round trip", back.Port) + } +} + +func TestUseSettingsFile(t *testing.T) { + defer func() { settingsFile = "" }() + path := filepath.Join(t.TempDir(), "elsewhere.json") + UseSettingsFile(path) + + if err := SaveSettings(Settings{Bridges: []Bridge{{ID: "bridge-abc123", Name: "Work"}}}); err != nil { + t.Fatalf("SaveSettings: %v", err) + } + got, err := LoadSettings() + if err != nil { + t.Fatalf("LoadSettings: %v", err) + } + if len(got.Bridges) != 1 || got.Bridges[0].Name != "Work" { + t.Errorf("bridges = %+v, want the one saved at the override path", got.Bridges) + } + p, err := settingsPath() + if err != nil { + t.Fatal(err) + } + if p != path { + t.Errorf("settingsPath = %q, want %q", p, path) + } +} + +// An endpoint list written before names existed must load with empty names, +// and a name must survive the round trip. +func TestEndpointNameRoundTrip(t *testing.T) { + var old endpointList + if err := old.UnmarshalJSON([]byte(`[{"url":"http://ai"}]`)); err != nil { + t.Fatal(err) + } + if len(old) != 1 || old[0].Name != "" || old[0].Endpoint != Direct("http://ai") { + t.Errorf("old record = %#v", old) + } + list := endpointList{{Endpoint: Direct("http://ai"), Name: "e1"}} + data, err := json.Marshal(list) + if err != nil { + t.Fatal(err) + } + if string(data) != `[{"name":"e1","url":"http://ai"}]` { + t.Errorf("marshal = %s", data) + } + var back endpointList + if err := back.UnmarshalJSON(data); err != nil { + t.Fatal(err) + } + if len(back) != 1 || back[0].Name != "e1" || back[0].Endpoint != Direct("http://ai") { + t.Errorf("decoded = %#v", back) + } +} diff --git a/internal/config/startup.go b/internal/config/startup.go index 5d85867..62ce2cb 100644 --- a/internal/config/startup.go +++ b/internal/config/startup.go @@ -9,25 +9,31 @@ import ( // to open. When neither flag names anything it returns the saved active // Endpoint. // -// A URL alone means a direct connection. A bridge alone opens at -// DefaultLocation, the same guess the connection picker makes, because naming -// a bridge usually means knowing how to get on the tailnet rather than what -// is listening on it. Both together pin the URL behind the bridge. +// The endpoint value resolves name first: a value matching a saved endpoint +// name is that endpoint, and anything else parses as a URL. A URL alone +// means a direct connection. A bridge alone opens at DefaultLocation, the +// same guess the connection picker makes, because naming a bridge usually +// means knowing how to get on the tailnet rather than what is listening on +// it. Both together pin the URL behind the bridge. With a named endpoint +// the bridge flag may only confirm the bridge the endpoint already uses. // // Callers resolve the flags before the TUI takes the terminal, so a URL we // cannot use becomes a line on stderr rather than a full-screen error. -func EndpointFromFlags(g *Global, rawURL, bridgeName string) (Endpoint, error) { - rawURL = strings.TrimSpace(rawURL) +func EndpointFromFlags(g *Global, rawEndpoint, bridgeName string) (Endpoint, error) { + rawEndpoint = strings.TrimSpace(rawEndpoint) bridgeName = strings.TrimSpace(bridgeName) - if rawURL == "" && bridgeName == "" { + if rawEndpoint == "" && bridgeName == "" { return g.ActiveEndpoint(), nil } + if named, ok := g.Settings.EndpointNamed(rawEndpoint); ok { + return checkBridgeFlag(g, named, bridgeName) + } // Check the URL before looking up the bridge, because the lookup writes. // An invocation that exits with a usage error must not leave a bridge on // disk that the user then has to find and delete. location := DefaultLocation - if rawURL != "" { - parsed, err := ParseEndpointURL(rawURL) + if rawEndpoint != "" { + parsed, err := ParseEndpointURL(rawEndpoint) if err != nil { return nil, err } @@ -43,6 +49,25 @@ func EndpointFromFlags(g *Global, rawURL, bridgeName string) (Endpoint, error) { return Bridged(location, bridge.ID), nil } +// checkBridgeFlag verifies a bridge flag against a named endpoint. The flag +// may only confirm the bridge the endpoint already uses; anything else is a +// contradiction on the command line, not a new endpoint. No bridge is +// created here: a usage error must not write. +func checkBridgeFlag(g *Global, ep Endpoint, bridgeName string) (Endpoint, error) { + if bridgeName == "" { + return ep, nil + } + bridged, ok := ep.(BridgeEndpoint) + if !ok { + return nil, fmt.Errorf("the endpoint is direct; --bridge %q does not apply", bridgeName) + } + bridge, ok := g.Bridge(bridged.BridgeID()) + if !ok || !strings.EqualFold(bridge.Name, bridgeName) { + return nil, fmt.Errorf("the endpoint does not go through bridge %q", bridgeName) + } + return ep, nil +} + // bridgeNamed returns the bridge called name, creating it when there is none. // Creating it is what makes a first run scriptable. Matching ignores case. // The name is the user's own label and nothing keys off it. @@ -59,7 +84,7 @@ func bridgeNamed(g *Global, name string) (Bridge, error) { } switch len(matched) { case 0: - return g.AddBridge(name) + return g.AddBridge(name, 0) case 1: return matched[0], nil } diff --git a/internal/config/startup_test.go b/internal/config/startup_test.go index 7ad4c4e..bc4a248 100644 --- a/internal/config/startup_test.go +++ b/internal/config/startup_test.go @@ -26,7 +26,7 @@ func loadInto(t *testing.T, s config.Settings) *config.Global { } func TestResolveFallsBackToTheSavedOne(t *testing.T) { - g := loadInto(t, config.Settings{Endpoints: []config.Endpoint{config.Direct("http://saved")}}) + g := loadInto(t, config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://saved")}}}) ep, err := config.EndpointFromFlags(g, "", "") if err != nil { @@ -38,7 +38,7 @@ func TestResolveFallsBackToTheSavedOne(t *testing.T) { } func TestResolveTakesABareHost(t *testing.T) { - g := loadInto(t, config.Settings{Endpoints: []config.Endpoint{config.Direct("http://saved")}}) + g := loadInto(t, config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://saved")}}}) ep, err := config.EndpointFromFlags(g, "aperture.example.com", "") if err != nil { @@ -140,3 +140,146 @@ func TestResolveRejectsAnAmbiguousBridgeName(t *testing.T) { } } } + +func TestAddBridgeStoresThePort(t *testing.T) { + g := loadInto(t, config.Settings{}) + + b, err := g.AddBridge("Work", 8991) + if err != nil { + t.Fatalf("AddBridge: %v", err) + } + if b.Port != 8991 { + t.Errorf("port = %d, want 8991", b.Port) + } + saved, err := config.LoadSettings() + if err != nil { + t.Fatalf("LoadSettings: %v", err) + } + if len(saved.Bridges) != 1 || saved.Bridges[0].Port != 8991 { + t.Errorf("saved bridges = %+v, want the port written out", saved.Bridges) + } +} + +// Bridge names are the command-line handle, so two bridges may not share one. +// Duplicates from hand-edited files still error at resolution instead. +func TestAddBridgeRefusesADuplicateName(t *testing.T) { + g := loadInto(t, config.Settings{Bridges: []config.Bridge{{ID: "bridge-abc123", Name: "Work"}}}) + + if _, err := g.AddBridge("work", 0); err == nil { + t.Error("AddBridge took a second bridge called work") + } + if len(g.Settings.Bridges) != 1 { + t.Errorf("bridges = %+v, want the duplicate refused in memory too", g.Settings.Bridges) + } +} + +func TestAddBridgeRefusesABadPort(t *testing.T) { + g := loadInto(t, config.Settings{}) + + for _, port := range []int{-1, 65536} { + if _, err := g.AddBridge("Work", port); err == nil { + t.Errorf("AddBridge took port %d", port) + } + } +} + +func TestNameEndpoint(t *testing.T) { + g := loadInto(t, config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://ai")}}}) + + if err := g.NameEndpoint(config.Direct("http://ai"), "e1"); err != nil { + t.Fatalf("NameEndpoint: %v", err) + } + if len(g.Settings.Endpoints) != 1 || g.Settings.Endpoints[0].Name != "e1" { + t.Fatalf("endpoints = %+v, want e1 named in place", g.Settings.Endpoints) + } + + // A second destination gets its own row; a taken name and a URL-looking + // name are refused. + if err := g.NameEndpoint(config.Direct("http://other"), "E1"); err == nil { + t.Error("NameEndpoint took a name another endpoint holds") + } + if err := g.NameEndpoint(config.Direct("http://other"), "http://ai"); err == nil { + t.Error("NameEndpoint took a name that parses as a URL") + } + if err := g.NameEndpoint(config.Direct("http://other"), "e2"); err != nil { + t.Fatalf("NameEndpoint: %v", err) + } + if len(g.Settings.Endpoints) != 2 || g.Settings.Endpoints[1].Name != "e2" { + t.Errorf("endpoints = %+v, want e2 appended", g.Settings.Endpoints) + } + + saved, err := config.LoadSettings() + if err != nil { + t.Fatalf("LoadSettings: %v", err) + } + if len(saved.Endpoints) != 2 || saved.Endpoints[0].Name != "e1" || saved.Endpoints[1].Name != "e2" { + t.Errorf("saved endpoints = %+v, want both names written out", saved.Endpoints) + } +} + +func TestResolveTakesAnEndpointName(t *testing.T) { + g := loadInto(t, config.Settings{ + Bridges: []config.Bridge{{ID: "bridge-abc123", Name: "Work"}}, + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://ai"), Name: "e1"}, + {Endpoint: config.Bridged("http://aperture.example.com", "bridge-abc123"), Name: "e2"}, + }, + }) + + // Names match without case, and a named bridged endpoint keeps its bridge. + ep, err := config.EndpointFromFlags(g, "E1", "") + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if ep != (config.Direct("http://ai")) { + t.Errorf("endpoint = %+v, want the one named e1", ep) + } + ep, err = config.EndpointFromFlags(g, "e2", "") + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if ep != (config.Bridged("http://aperture.example.com", "bridge-abc123")) { + t.Errorf("endpoint = %+v, want the one named e2 with its bridge", ep) + } + + // The bridge flag may confirm the bridge a named endpoint uses, but not + // contradict it, and it means nothing against a direct endpoint. + if _, err := config.EndpointFromFlags(g, "e2", "work"); err != nil { + t.Errorf("Resolve refused the bridge e2 already uses: %v", err) + } + if _, err := config.EndpointFromFlags(g, "e2", "other"); err == nil { + t.Error("Resolve moved e2 to a bridge it does not use") + } + if _, err := config.EndpointFromFlags(g, "e1", "work"); err == nil { + t.Error("Resolve pinned a bridge onto a direct endpoint") + } + if len(g.Settings.Bridges) != 1 { + t.Errorf("bridges = %+v, want rejected combinations to create nothing", g.Settings.Bridges) + } +} + +// A name that matches nothing is a host, which is what bare hosts have always +// meant on this flag. +func TestResolveFallsBackToAURLForAnUnknownName(t *testing.T) { + g := loadInto(t, config.Settings{}) + + ep, err := config.EndpointFromFlags(g, "e1", "") + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if ep != (config.Direct("http://e1")) { + t.Errorf("endpoint = %+v, want e1 read as a bare host", ep) + } +} + +// Names are tab-separated output a script parses and a status line a user +// reads, so a newline would forge a row. +func TestRefusesControlCharactersInNames(t *testing.T) { + g := loadInto(t, config.Settings{}) + if _, err := g.AddBridge("bad\tbridge", 0); err == nil { + t.Error("AddBridge took a tab in a name") + } + if err := g.NameEndpoint(config.Direct("http://ai"), "bad\nendpoint"); err == nil { + t.Error("NameEndpoint took a newline in a name") + } +} diff --git a/internal/config/state_test.go b/internal/config/state_test.go index 9745875..4edca73 100644 --- a/internal/config/state_test.go +++ b/internal/config/state_test.go @@ -82,8 +82,8 @@ func TestGlobal_RecordLaunchBindsActiveEndpoint(t *testing.T) { t.Setenv("HOME", tmp) t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) - g := &config.Global{Settings: config.Settings{Endpoints: []config.Endpoint{ - config.Bridged("http://aperture.example.com", "bridge-abcdef"), + g := &config.Global{Settings: config.Settings{Endpoints: []config.EndpointEntry{ + {Endpoint: config.Bridged("http://aperture.example.com", "bridge-abcdef")}, }}} if err := g.RecordLaunch(config.LaunchState{LastClientName: "Claude Code"}); err != nil { t.Fatal(err) @@ -102,9 +102,9 @@ func TestSettings_RoundTrip(t *testing.T) { Bridges: []config.Bridge{ {ID: "bridge-abcdef", Name: "Work"}, }, - Endpoints: []config.Endpoint{ - config.Direct("http://ai"), - config.Bridged("http://aperture.example.com", "bridge-abcdef"), + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://ai")}, + {Endpoint: config.Bridged("http://aperture.example.com", "bridge-abcdef")}, }, YoloMode: true, } @@ -116,13 +116,13 @@ func TestSettings_RoundTrip(t *testing.T) { if err != nil { t.Fatalf("LoadSettings: %v", err) } - if len(got.Endpoints) != 2 || got.Endpoints[0].URL() != "http://ai" { + if len(got.Endpoints) != 2 || got.Endpoints[0].Endpoint.URL() != "http://ai" { t.Errorf("endpoints = %+v", got.Endpoints) } if len(got.Bridges) != 1 || got.Bridges[0].ID != "bridge-abcdef" { t.Errorf("bridges = %+v", got.Bridges) } - if got.Endpoints[1] != config.Endpoint(config.Bridged("http://aperture.example.com", "bridge-abcdef")) { + if got.Endpoints[1].Endpoint != config.Endpoint(config.Bridged("http://aperture.example.com", "bridge-abcdef")) { t.Errorf("bridge endpoint = %+v", got.Endpoints[1]) } if !got.YoloMode { @@ -139,7 +139,7 @@ func TestSettings_MissingFileUsesDefaults(t *testing.T) { if err != nil { t.Fatal(err) } - if len(got.Endpoints) != 1 || got.Endpoints[0].URL() != config.DefaultLocation { + if len(got.Endpoints) != 1 || got.Endpoints[0].Endpoint.URL() != config.DefaultLocation { t.Errorf("default endpoints = %+v", got.Endpoints) } } @@ -183,10 +183,10 @@ func TestGlobal_SetApertureHost_RotatesToFront(t *testing.T) { g := &config.Global{ Settings: config.Settings{ - Endpoints: []config.Endpoint{ - config.Direct("http://a"), - config.Direct("http://b"), - config.Direct("http://c"), + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://a")}, + {Endpoint: config.Direct("http://b")}, + {Endpoint: config.Direct("http://c")}, }, }, } @@ -196,8 +196,8 @@ func TestGlobal_SetApertureHost_RotatesToFront(t *testing.T) { if g.ApertureHost != "http://b" { t.Errorf("ApertureHost = %q", g.ApertureHost) } - if g.Settings.Endpoints[0].URL() != "http://b" { - t.Errorf("front endpoint = %q, want http://b", g.Settings.Endpoints[0].URL()) + if g.Settings.Endpoints[0].Endpoint.URL() != "http://b" { + t.Errorf("front endpoint = %q, want http://b", g.Settings.Endpoints[0].Endpoint.URL()) } if len(g.Settings.Endpoints) != 3 { t.Errorf("endpoints len = %d, want 3", len(g.Settings.Endpoints)) @@ -211,16 +211,16 @@ func TestGlobal_SetActiveEndpoint_DistinguishesBridge(t *testing.T) { g := &config.Global{ Settings: config.Settings{ - Endpoints: []config.Endpoint{ - config.Direct("http://ai"), - config.Bridged("http://ai", "bridge-abcdef"), + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://ai")}, + {Endpoint: config.Bridged("http://ai", "bridge-abcdef")}, }, }, } if err := g.SetActiveEndpoint(config.Bridged("http://ai", "bridge-abcdef"), nil); err != nil { t.Fatal(err) } - if g.Settings.Endpoints[0] != config.Endpoint(config.Bridged("http://ai", "bridge-abcdef")) { + if g.Settings.Endpoints[0].Endpoint != config.Endpoint(config.Bridged("http://ai", "bridge-abcdef")) { t.Errorf("front endpoint = %+v, want bridge endpoint", g.Settings.Endpoints[0]) } if len(g.Settings.Endpoints) != 2 { @@ -228,6 +228,30 @@ func TestGlobal_SetActiveEndpoint_DistinguishesBridge(t *testing.T) { } } +func TestGlobal_SetActiveEndpoint_URLEditKeepsName(t *testing.T) { + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) + + g := &config.Global{ + Settings: config.Settings{ + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://old"), Name: "work"}, + {Endpoint: config.Direct("http://new"), Name: ""}, + }, + }, + } + if err := g.SetActiveEndpoint(config.Direct("http://new"), config.Direct("http://old")); err != nil { + t.Fatal(err) + } + if g.Settings.Endpoints[0].Name != "work" { + t.Errorf("front endpoint name = %q, want %q", g.Settings.Endpoints[0].Name, "work") + } + if g.Settings.Endpoints[0].Endpoint.URL() != "http://new" { + t.Errorf("front endpoint url = %q, want http://new", g.Settings.Endpoints[0].Endpoint.URL()) + } +} + func TestGlobal_RemoveInactiveEndpointPreservesRuntimeHost(t *testing.T) { tmp := t.TempDir() t.Setenv("HOME", tmp) @@ -235,9 +259,9 @@ func TestGlobal_RemoveInactiveEndpointPreservesRuntimeHost(t *testing.T) { g := &config.Global{ ApertureHost: "http://127.0.0.1:12345", - Settings: config.Settings{Endpoints: []config.Endpoint{ - config.Bridged("http://active", "bridge-abcdef"), - config.Bridged("http://candidate", "bridge-fedcba"), + Settings: config.Settings{Endpoints: []config.EndpointEntry{ + {Endpoint: config.Bridged("http://active", "bridge-abcdef")}, + {Endpoint: config.Bridged("http://candidate", "bridge-fedcba")}, }}, } if err := g.RemoveEndpoint(config.Bridged("http://candidate", "bridge-fedcba")); err != nil { @@ -255,11 +279,11 @@ func TestGlobal_ReplaceEndpointDeduplicates(t *testing.T) { active := config.Direct("http://active") candidate := config.Direct("http://candidate") - g := &config.Global{Settings: config.Settings{Endpoints: []config.Endpoint{active, candidate}}} + g := &config.Global{Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: active}, {Endpoint: candidate}}}} if err := g.ReplaceEndpoint(candidate, active); err != nil { t.Fatal(err) } - if len(g.Settings.Endpoints) != 1 || g.Settings.Endpoints[0] != active { + if len(g.Settings.Endpoints) != 1 || g.Settings.Endpoints[0].Endpoint != active { t.Fatalf("endpoints = %+v, want one active endpoint", g.Settings.Endpoints) } } diff --git a/internal/profiles/adapter.go b/internal/profiles/adapter.go index d29a5cc..e6e6e0a 100644 --- a/internal/profiles/adapter.go +++ b/internal/profiles/adapter.go @@ -1,6 +1,7 @@ package profiles import ( + "errors" "os/exec" "runtime" @@ -93,6 +94,12 @@ func (c *desktopClient) Replay(g *config.Global) tea.Cmd { return res.Cmd } +// Spec implements clients.Client. Claude Cowork is a desktop app, not a +// command-line agent, so a headless launch has no meaning for it. +func (c *desktopClient) Spec(g *config.Global, provider, model string, args []string) (clients.LaunchSpec, error) { + return clients.LaunchSpec{}, errors.New(desktopName + " is a desktop app; configure it from the interactive launcher") +} + func (c *desktopClient) QuickSelectLabel(g *config.Global) string { return desktopName + " (Anthropic via Claude Cowork)" } diff --git a/internal/tui/connection_test.go b/internal/tui/connection_test.go index 2327b9e..8577d25 100644 --- a/internal/tui/connection_test.go +++ b/internal/tui/connection_test.go @@ -48,7 +48,7 @@ func TestEndpointEditPreservesVerifiedConnection(t *testing.T) { if err != nil { t.Fatal(err) } - if saved.Endpoints[0] != old { + if saved.Endpoints[0].Endpoint != old { t.Errorf("%s persisted unverified active endpoint: %+v", outcome, saved.Endpoints) } }) @@ -78,9 +78,9 @@ func TestTailnetSwitchInvalidatesSharedConnection(t *testing.T) { m := pickerModel(t) withFakeClients(t, []clients.Client{&fakeClient{name: "Test agent", installed: true}}) withFakeTailscale(t, tsConnected) - target := m.g.Settings.Endpoints[1] + target := m.g.Settings.Endpoints[1].Endpoint if shared { - m.g.Settings.Endpoints[0] = config.Bridged("http://first", target.(config.BridgeEndpoint).BridgeID()) + m.g.Settings.Endpoints[0] = config.EndpointEntry{Endpoint: config.Bridged("http://first", target.(config.BridgeEndpoint).BridgeID())} } m.g.ApertureHost = "http://127.0.0.1:12345" m.resetStack(m.rootMenu()) @@ -197,8 +197,8 @@ func TestEndpointEditSameCandidateCancellation(t *testing.T) { if err != nil { t.Fatal(err) } - for _, ep := range saved.Endpoints { - if ep == candidate { + for _, entry := range saved.Endpoints { + if entry.Endpoint == candidate { t.Error("cancelled candidate remains on disk") } } diff --git a/internal/tui/menus.go b/internal/tui/menus.go index 5368322..7c64de7 100644 --- a/internal/tui/menus.go +++ b/internal/tui/menus.go @@ -3,7 +3,6 @@ package tui import ( "fmt" "os" - "slices" "strings" tea "github.com/charmbracelet/bubbletea" @@ -139,7 +138,7 @@ func (m *model) lastLaunchEndpoint() (config.Endpoint, bool) { } func (m *model) endpointConfigured(want config.Endpoint) bool { - return slices.Contains(m.g.Settings.Endpoints, want) + return m.g.Settings.HasEndpoint(want) } func simpleErrorCmd(err error) tea.Cmd { @@ -200,7 +199,7 @@ func (m *model) bridgesMenu() *menu.Menu { Hidden: true, Action: func() menu.Result { m.promptForInput("Add Bridge:", "Name", "", func(v string) tea.Cmd { - if _, err := m.g.AddBridge(v); err != nil { + if _, err := m.g.AddBridge(v, 0); err != nil { return func() tea.Msg { return menu.SimpleDoneMsg{Err: err} } } m.refreshBridgesMenu() @@ -324,9 +323,9 @@ type connectionRow struct { func (m *model) connectionRows() []connectionRow { rows := make([]connectionRow, 0, len(m.g.Settings.Endpoints)+len(m.g.Settings.Bridges)) used := make(map[string]bool, len(m.g.Settings.Bridges)) - for i, ep := range m.g.Settings.Endpoints { - row := connectionRow{ep: ep, saved: true, active: i == 0} - if bridged, ok := ep.(config.BridgeEndpoint); ok { + for i, entry := range m.g.Settings.Endpoints { + row := connectionRow{ep: entry.Endpoint, saved: true, active: i == 0} + if bridged, ok := entry.Endpoint.(config.BridgeEndpoint); ok { used[bridged.BridgeID()] = true row.bridge, _ = m.g.Bridge(bridged.BridgeID()) } @@ -661,7 +660,7 @@ func (m *model) endpointBridgeMenu() *menu.Menu { Label: "Add Bridge", Action: func() menu.Result { m.promptForInput("Add Bridge:", "Name", "", func(v string) tea.Cmd { - bridge, err := m.g.AddBridge(v) + bridge, err := m.g.AddBridge(v, 0) if err != nil { return simpleErrorCmd(err) } diff --git a/internal/tui/removal.go b/internal/tui/removal.go index 5ebd2cc..61cf1d1 100644 --- a/internal/tui/removal.go +++ b/internal/tui/removal.go @@ -2,6 +2,7 @@ package tui import ( "context" + "log/slog" "strings" "time" @@ -143,6 +144,7 @@ func (m *model) bridgeRemoved(msg bridgeRemovedMsg) (tea.Model, tea.Cmd) { err = bridges.RemoveFromSettings(m.g, msg.bridge, msg.endpoint) } if err != nil { + slog.Error("bridge removal failed", "bridge", msg.bridge.ID, "err", err) m.quitAfterRemoval = false m.step = stepError m.errMsg = m.removalFailedMessage(msg.bridge, err) diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 252f12e..ceffa45 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -436,6 +436,7 @@ func (m *model) cancelActivation() (tea.Model, tea.Cmd) { } endpoint := act.endpoint() if err := m.discardActivation(); err != nil { + slog.Error("discarding activation failed", "err", err) m.errMsg = "could not remove endpoint: " + err.Error() m.step = stepError return m, nil @@ -484,6 +485,7 @@ func (m *model) retargetActivation(next config.Endpoint) tea.Cmd { m.stopActivation() a, err := act.attempt.Retarget(m.g, next) if err != nil { + slog.Error("retargeting activation failed", "err", err) m.errMsg = err.Error() m.step = stepError return nil @@ -587,6 +589,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.act.cancel = nil a := m.act.attempt if msg.err != nil { + slog.Error("endpoint activation failed", "err", msg.err) if a.TargetsActive { m.connected = false } @@ -598,6 +601,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } if err := a.Commit(m.g, msg.gateway); err != nil { + slog.Error("endpoint commit failed", "err", err) m.preflightErr = err.Error() m.forcedToEndpoint = true m.failedEndpoint = a.Endpoint @@ -685,6 +689,9 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Quit case menu.ExecDoneMsg: + if msg.Err != nil { + slog.Error("client process exited with error", "err", msg.Err) + } // A client's foreground launch has exited. Re-run preflight: the // user may have changed things outside the launcher while the // agent was running. @@ -698,6 +705,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case menu.InstallDoneMsg: if msg.Err != nil { + slog.Error("client install failed", "err", msg.Err) m.errMsg = "Install failed: " + msg.Err.Error() m.step = stepError return m, nil @@ -708,6 +716,12 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.ClearScreen case menu.LaunchDoneMsg: + if msg.Err != nil { + slog.Error("desktop launch failed", "err", msg.Err) + m.errMsg = "Launch failed: " + msg.Err.Error() + m.step = stepError + return m, nil + } // Desktop-style launch returned immediately; stay on root menu. m.popToRoot() m.step = stepMenu @@ -716,6 +730,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case menu.SimpleDoneMsg: if msg.Err != nil { + slog.Error("menu action failed", "err", msg.Err) m.errMsg = msg.Err.Error() m.step = stepError return m, nil diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index 92380fe..0e86fa9 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -47,6 +47,9 @@ func (c *fakeClient) Menu(*config.Global) menu.MenuItem { } func (c *fakeClient) Replay(*config.Global) tea.Cmd { return c.replayCmd } func (c *fakeClient) QuickSelectLabel(*config.Global) string { return c.quickLabel } +func (c *fakeClient) Spec(*config.Global, string, string, []string) (clients.LaunchSpec, error) { + return clients.LaunchSpec{}, nil +} // withFakeClients swaps the TUI's client registry for the duration of the test. func withFakeClients(t *testing.T, cs []clients.Client) { @@ -90,7 +93,7 @@ func TestRootMenu_QuickSelectPrepended(t *testing.T) { withFakeClients(t, []clients.Client{fc}) m := &model{g: &config.Global{ - Settings: config.Settings{Endpoints: []config.Endpoint{config.Direct("http://ai")}}, + Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://ai")}}}, LastLaunch: config.LaunchState{ LastClientName: "A", LastEndpointURL: "http://ai", @@ -130,7 +133,7 @@ func TestRootMenu_NoQuickSelectWhenReplayNil(t *testing.T) { withFakeClients(t, []clients.Client{fc}) m := &model{g: &config.Global{ - Settings: config.Settings{Endpoints: []config.Endpoint{config.Direct("http://ai")}}, + Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://ai")}}}, LastLaunch: config.LaunchState{ LastClientName: "A", LastEndpointURL: "http://ai", @@ -155,7 +158,7 @@ func TestRootMenu_NoQuickSelectWithoutRecordedEndpoint(t *testing.T) { withFakeClients(t, []clients.Client{fc}) m := &model{g: &config.Global{ - Settings: config.Settings{Endpoints: []config.Endpoint{config.Direct("http://ai")}}, + Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://ai")}}}, LastLaunch: config.LaunchState{LastClientName: "A"}, }} m.connected = true @@ -183,7 +186,7 @@ func TestQuickSelectRequiresRecordedEndpointToBeActive(t *testing.T) { saved := config.Bridged("http://ai", "bridge-saved") m := &model{ g: &config.Global{ - Settings: config.Settings{Endpoints: []config.Endpoint{active, saved}}, + Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: active}, {Endpoint: saved}}}, Providers: []config.ProviderInfo{{ID: "old-provider"}}, LastLaunch: config.LaunchState{ LastClientName: "A", @@ -201,7 +204,7 @@ func TestQuickSelectRequiresRecordedEndpointToBeActive(t *testing.T) { // Switching back to the recorded URL and bridge makes the saved launch // valid again without rewriting launch state. - m.g.Settings.Endpoints = []config.Endpoint{saved, active} + m.g.Settings.Endpoints = []config.EndpointEntry{{Endpoint: saved}, {Endpoint: active}} quick := m.rootMenu().Items[0] if !strings.Contains(quick.Label, "Quick select") || quick.Action().Cmd == nil { t.Fatalf("quick-select did not reappear for active recorded endpoint: %+v", quick) @@ -514,7 +517,7 @@ func TestSetupGuideMenu_BridgeDoesNotRequireSystemTailscale(t *testing.T) { ApertureHost: "http://aperture", Settings: config.Settings{ Bridges: []config.Bridge{{ID: "bridge-abcdef", Name: "Work Bridge"}}, - Endpoints: []config.Endpoint{config.Bridged("http://aperture", "bridge-abcdef")}, + Endpoints: []config.EndpointEntry{{Endpoint: config.Bridged("http://aperture", "bridge-abcdef")}}, }, }} @@ -533,7 +536,7 @@ func TestEndpointBridgeMenu_AddsFirstBridgeInline(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", tmp+"/.config") m := &model{ g: &config.Global{Settings: config.Settings{ - Endpoints: []config.Endpoint{config.Direct("http://other")}, + Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://other")}}, }}, step: stepMenu, } @@ -593,7 +596,7 @@ func TestEndpointBridgeMenu_ConnectsExistingBridgeWithoutPrompting(t *testing.T) m := &model{ g: &config.Global{Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{config.Direct("http://other")}, + Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://other")}}, }}, step: stepMenu, } @@ -629,7 +632,7 @@ func TestInitOpensOnTheStartEndpoint(t *testing.T) { named := config.Bridged("http://named", bridge.ID) m := NewModel(&config.Global{Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{saved}, + Endpoints: []config.EndpointEntry{{Endpoint: saved}}, }}, "B0-test", nil, named).(*model) if cmd := m.Init(); cmd == nil { @@ -653,7 +656,7 @@ func TestInitOpensOnTheSavedEndpointWhenNothingIsNamed(t *testing.T) { t.Setenv("HOME", tmp) t.Setenv("XDG_CONFIG_HOME", tmp+"/.config") saved := config.Direct("http://saved") - g := &config.Global{Settings: config.Settings{Endpoints: []config.Endpoint{saved}}} + g := &config.Global{Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: saved}}}} m := NewModel(g, "B0-test", nil, g.ActiveEndpoint()).(*model) if cmd := m.Init(); cmd == nil { @@ -676,7 +679,7 @@ func TestPreflightOverrideReplacesGuessedEndpoint(t *testing.T) { m := &model{ g: &config.Global{Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{previous}, + Endpoints: []config.EndpointEntry{{Endpoint: previous}}, }}, step: stepMenu, } @@ -703,7 +706,7 @@ func TestPreflightOverrideReplacesGuessedEndpoint(t *testing.T) { t.Error("override reused the cancelled attempt's id, so its stale result would be applied") } // The guess is replaced, not accumulated, and the working endpoint stays. - if got := m.g.Settings.Endpoints; len(got) != 2 || got[0] != previous || got[1] != want { + if got := m.g.Settings.Endpoints; len(got) != 2 || got[0].Endpoint != previous || got[1].Endpoint != want { t.Fatalf("endpoints = %+v, want the previous one plus the typed one", got) } } @@ -716,7 +719,7 @@ func TestPreflightOverrideRejectsBadURLWithoutStoppingTheAttempt(t *testing.T) { m := &model{ g: &config.Global{Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{config.Direct("http://other")}, + Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://other")}}, }}, step: stepMenu, } @@ -770,7 +773,7 @@ func TestPreflightEscapeAbandonsDiscovery(t *testing.T) { m := &model{ g: &config.Global{Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{previous}, + Endpoints: []config.EndpointEntry{{Endpoint: previous}}, }}, step: stepMenu, } @@ -786,7 +789,7 @@ func TestPreflightEscapeAbandonsDiscovery(t *testing.T) { if m.step != stepMenu || m.top().Title != "Choose a bridge" { t.Fatalf("Esc did not return to the bridge chooser: step=%v top=%+v", m.step, m.top()) } - if got := m.g.Settings.Endpoints; len(got) != 1 || got[0] != previous { + if got := m.g.Settings.Endpoints; len(got) != 1 || got[0].Endpoint != previous { t.Fatalf("endpoints = %+v, want the abandoned guess removed", got) } // A late result from the abandoned attempt must not take over the screen. @@ -800,7 +803,7 @@ func TestPreflightEscapeAtStartupShowsSetupGuide(t *testing.T) { withFakeTailscale(t, tsConnected) m := &model{g: &config.Global{ ApertureHost: "http://ai", - Settings: config.Settings{Endpoints: []config.Endpoint{config.Direct("http://ai")}}, + Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: config.Direct("http://ai")}}}, }} m.Init() @@ -846,7 +849,7 @@ func TestEndpointsMenu_EditRetargetsWorkingEndpoint(t *testing.T) { ApertureHost: connected.URL(), Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{connected}, + Endpoints: []config.EndpointEntry{{Endpoint: connected}}, }, }, step: stepMenu, @@ -880,7 +883,7 @@ func TestEndpointsMenu_EditRetargetsWorkingEndpoint(t *testing.T) { t.Fatalf("activation = %+v, want a connection to %+v", m.act, want) } m.Update(endpointActivationResult{id: m.act.id, gateway: bridges.Gateway{URL: "http://127.0.0.1:12345"}}) - if got := m.g.Settings.Endpoints; len(got) != 1 || got[0] != want { + if got := m.g.Settings.Endpoints; len(got) != 1 || got[0].Endpoint != want { t.Fatalf("endpoints = %+v, want verified replacement %+v", got, want) } } @@ -902,9 +905,9 @@ func pickerModel(t *testing.T) *model { {ID: "bridge-aaaaaa", Name: "Work", Tailnet: "corp.example.com"}, {ID: "bridge-bbbbbb", Name: "Home"}, }, - Endpoints: []config.Endpoint{ - config.Direct(config.DefaultLocation), - config.Bridged(config.DefaultLocation, "bridge-aaaaaa"), + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct(config.DefaultLocation)}, + {Endpoint: config.Bridged(config.DefaultLocation, "bridge-aaaaaa")}, }, }, }, @@ -1069,7 +1072,7 @@ func TestConnectionPicker_DeleteKeyRemovesRowUnderCursor(t *testing.T) { m.setCursor(1) // http://ai via Work del, _ = findItem(t, m.top().Items, "delete") m.activate(del) - if got := m.g.Settings.Endpoints; len(got) != 1 || got[0] != config.Endpoint(config.Direct(config.DefaultLocation)) { + if got := m.g.Settings.Endpoints; len(got) != 1 || got[0].Endpoint != config.Endpoint(config.Direct(config.DefaultLocation)) { t.Fatalf("endpoints = %+v, want the bridge endpoint removed", got) } } @@ -1114,7 +1117,7 @@ func TestSetupGuideExplainsDefaultLocationGuess(t *testing.T) { ApertureHost: config.DefaultLocation, Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{target}, + Endpoints: []config.EndpointEntry{{Endpoint: target}}, }, }, failedEndpoint: target, @@ -1151,7 +1154,7 @@ func TestBridgeEndpointFailureKeepsPreviousEndpointActive(t *testing.T) { ApertureHost: "http://old", Settings: config.Settings{ Bridges: []config.Bridge{bridge}, - Endpoints: []config.Endpoint{old}, + Endpoints: []config.EndpointEntry{{Endpoint: old}}, }, Providers: []config.ProviderInfo{{ID: "old-provider"}}, }, @@ -1209,7 +1212,7 @@ func TestDirectEndpointIsPromotedOnlyAfterModelsSucceed(t *testing.T) { m := &model{ g: &config.Global{ ApertureHost: "http://old", - Settings: config.Settings{Endpoints: []config.Endpoint{old}}, + Settings: config.Settings{Endpoints: []config.EndpointEntry{{Endpoint: old}}}, Providers: []config.ProviderInfo{{ID: "old-provider"}}, }, step: stepMenu, @@ -1573,7 +1576,7 @@ func TestFailureViewWrapsDiagnostics(t *testing.T) { Debug: true, Settings: config.Settings{ Bridges: []config.Bridge{{ID: "bridge-abcdef", Name: "Work Bridge"}}, - Endpoints: []config.Endpoint{config.Bridged("http://aperture.example.ts.net", "bridge-abcdef")}, + Endpoints: []config.EndpointEntry{{Endpoint: config.Bridged("http://aperture.example.ts.net", "bridge-abcdef")}}, }, }, width: 50, @@ -1610,7 +1613,7 @@ func TestRootHeaderShowsLogicalBridgeEndpoint(t *testing.T) { ApertureHost: "http://127.0.0.1:41234", Settings: config.Settings{ Bridges: []config.Bridge{{ID: "bridge-abcdef", Name: "Work"}}, - Endpoints: []config.Endpoint{config.Bridged("http://ai", "bridge-abcdef")}, + Endpoints: []config.EndpointEntry{{Endpoint: config.Bridged("http://ai", "bridge-abcdef")}}, }, }, step: stepMenu, @@ -1673,9 +1676,9 @@ func TestBridgeLogSinkNeverDropsTheLoginLink(t *testing.T) { func TestRemoveConnectionRowTakesTheBridgeWithIt(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) m := &model{g: &config.Global{Settings: config.Settings{ - Endpoints: []config.Endpoint{ - config.Direct("http://active"), - config.Bridged("http://ai", "b1"), + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://active")}, + {Endpoint: config.Bridged("http://ai", "b1")}, }, Bridges: []config.Bridge{{ID: "b1", Name: "work"}}, }}} @@ -1703,10 +1706,10 @@ func TestRemoveConnectionRowTakesTheBridgeWithIt(t *testing.T) { func TestRemoveConnectionRowKeepsASharedBridge(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) m := &model{g: &config.Global{Settings: config.Settings{ - Endpoints: []config.Endpoint{ - config.Direct("http://active"), - config.Bridged("http://ai", "b1"), - config.Bridged("http://other", "b1"), + Endpoints: []config.EndpointEntry{ + {Endpoint: config.Direct("http://active")}, + {Endpoint: config.Bridged("http://ai", "b1")}, + {Endpoint: config.Bridged("http://other", "b1")}, }, Bridges: []config.Bridge{{ID: "b1", Name: "work"}}, }}} diff --git a/plans/headless-cli-and-serve.md b/plans/headless-cli-and-serve.md new file mode 100644 index 0000000..853d703 --- /dev/null +++ b/plans/headless-cli-and-serve.md @@ -0,0 +1,74 @@ +# Headless CLI and persistent serve + +A command grammar for aperture alongside the TUI: config subcommands that +mutate settings.json, direct client launch with argument passthrough, and +`serve`, a persistent fixed-port bridge that replaces the ts-unplug sidecar. +No arguments keeps the TUI. Specs and ADRs land before code. + +Decisions are [ADR 0007](../docs/adr/0007-headless-cli.md) (grammar, cobra, +passthrough, naming, `--config`) and +[ADR 0008](../docs/adr/0008-persistent-bridge-fixed-port.md) (`serve`, fixed +port on the Bridge record, no daemon). + +## Grammar + +``` +aperture # TUI, unchanged +aperture bridge add [--port N] +aperture bridge list +aperture bridge describe +aperture bridge delete +aperture endpoint add [--bridge ] +aperture endpoint list +aperture endpoint delete +aperture serve --bridge [--port N] [--endpoint ] +aperture [--endpoint ] [--bridge ] [--provider p] [--model m] [-- client args...] +aperture --config # or APERTURE_CONFIG +``` + +Endpoints and bridges are addressed by Name, with raw-URL fallback for +endpoints. In a client command, aperture consumes its own flags anywhere in +the tail; the first unknown flag or `--` starts verbatim passthrough, so +`alias codex='aperture codex'` works. + +## Steps (all landed) + +1. ADR 0007 and ADR 0008. +2. Specs: context map (Name and Port language rows, `bridge` command + disambiguation), domain model (Route fixed-port invariant), contracts + (command line becomes the API section; persisted facts gain Endpoint.Name + and Bridge.Port). +3. config: `endpointRecord` gains `name,omitempty`; lookup by name with URL + fallback; `Bridge` gains `Port int`; settings path override for + `--config`. Round-trip tests: old files load unchanged. +4. internal/cli: cobra command tree (cobra + pflag are the one new + dependency; Viper is rejected in ADR 0007). Root with no args runs the + TUI. Pure passthrough parser with table tests. Every flag carries a + single-letter shorthand, and a normalization pass keeps the deprecated + single-dash long forms (`-endpoint`) parsing (ADR 0007 amendment). Every + command's `--help` carries example invocations, with a test holding them + to the contract's grammar. +5. bridge and endpoint command groups. Delete goes through + `Machines.Destroy`. Name uniqueness enforced at add. +6. serve: `RouteTo` gains a listen port (0 keeps current behavior). Bind + failure errors fast. Login link prints to stderr. SIGTERM closes Machines. +7. clients: one headless launch entry on the Client interface; provider and + model empty means LastLaunch, then first compatible. Nine mechanical + implementations over the existing launch builders. Registry gains lookup + by binary name. +8. main.go: hand os.Args to the cobra root instead of flag.Parse. +9. e2e on the existing harness: command round-trips, headless launch of a + stub binary with passthrough asserted, serve on a fixed port. +10. README: the grammar, the alias recipe, a sample systemd user unit for + serve (documentation only). + +## Watch + +- pflag does not parse single-dash long flags, so a normalization pass + rewrites `-endpoint` to `--endpoint` before cobra runs. Deprecated in + docs and source, never rejected (ADR 0007 amendment). +- A bound serve port is a hard error, never a fallback to ephemeral. +- Login links go to stderr, never the run log. The redaction contract holds. +- `bridge delete` refuses while a serve holds the slot lock. That is + `Machines.Destroy`'s existing rule and the right one. +- `make check` is the gate.