Support Tailscale HTTPS Executor access - #18
Conversation
📝 WalkthroughWalkthroughThis PR adds per-box executor origin and network configuration, wires it through create/import/upgrade, introduces ChangesExecutor configurable origin and gateway CLI
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| Version: ver, | ||
| } | ||
|
|
||
| if !opts.NoStart { |
There was a problem hiding this comment.
🟠 High box/create.go:143
When Start fails after both containers are created, Create returns the error without removing the containers — the agent and executor are left behind in created state. This PR makes the failure path reachable in practice: opts.Executor.PublishPort is honored at create time, so Docker accepts the container, then Start fails if that host port is already bound. On the upgrade path the caller does not invoke Destroy on this error, so the newly created containers leak and must be removed manually before retrying. The agent-create failure path already best-effort removes the leaked container; the start-failure path at line 144 needs the same cleanup before returning.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/box/create.go around line 143:
When `Start` fails after both containers are created, `Create` returns the error without removing the containers — the agent and executor are left behind in `created` state. This PR makes the failure path reachable in practice: `opts.Executor.PublishPort` is honored at create time, so Docker accepts the container, then `Start` fails if that host port is already bound. On the upgrade path the caller does not invoke `Destroy` on this error, so the newly created containers leak and must be removed manually before retrying. The agent-create failure path already best-effort removes the leaked container; the start-failure path at line 144 needs the same cleanup before returning.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/box/executor_config_test.go (1)
104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing the on-disk delete path for
--clear-executor-config.This case only checks the in-memory
LoadExecutorConfig{Clear: true}result. It doesn't verify that callingSaveExecutorConfigwith the cleared config actually removes the previously-persistedTX9_EXECUTOR_*keys from the box env file (thesetOrDeletedelete branch inexecutor_config.go), which is the behavior--clear-executor-configusers actually rely on.Suggested addition
if err := SaveExecutorConfig(name, "secret-token", cfg); err != nil { t.Fatal(err) } env, err = state.ReadBoxEnv(name) if err != nil { t.Fatal(err) } if _, ok := env[ExecutorWebBaseURLEnv]; ok { t.Error("cleared WebBaseURL key was not removed from box env") } if _, ok := env[ExecutorPublishEnv]; ok { t.Error("cleared publish key was not removed from box env") } if _, ok := env[ExecutorDNSEnv]; ok { t.Error("cleared DNS key was not removed from box env") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/box/executor_config_test.go` around lines 104 - 111, The test for LoadExecutorConfig with Clear: true only verifies the in-memory cleared config and misses the on-disk delete behavior in SaveExecutorConfig/setOrDelete. Extend the existing executor_config_test case to save the cleared config back through SaveExecutorConfig, then read the box env and assert the previously persisted ExecutorWebBaseURLEnv, ExecutorPublishEnv, and ExecutorDNSEnv keys are removed. Use LoadExecutorConfig, SaveExecutorConfig, and state.ReadBoxEnv to exercise the full --clear-executor-config path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tx9-cli-design.md`:
- Line 49: The command-surface table entry for tx9 gateway currently uses
unescaped pipe characters, which will split the Markdown row into extra columns.
Update the table row in the docs/tx9-cli-design.md command list so the command
text renders as a single cell, either by escaping the pipe characters or
rephrasing the tx9 gateway <status|enable|disable> <box> syntax. Keep the fix
local to the affected table row.
In `@internal/box/box.go`:
- Around line 300-308: OpenURL currently swallows failures from url.Parse and
returns an empty string, which hides malformed webBaseURL problems from tx9
open. Update OpenURL to return both the URL string and an error, propagate the
parse error instead of discarding it, and adjust the caller(s) that use OpenURL
directly so they can handle and report the failure rather than opening a blank
target.
In `@internal/cli/cmd_doctor.go`:
- Around line 61-63: `probeExecutorPublicURL` is using a non-context-aware HTTP
call and triggers the `noctx` lint error; update it to accept `ctx` and build
the request with that context before calling the client so the probe can be
canceled. Propagate the new signature from the `cmd_doctor` probe call site, and
update the matching test call in `cmd_doctor_test.go` to pass a context (for
example `context.Background()`) so the API stays consistent.
---
Nitpick comments:
In `@internal/box/executor_config_test.go`:
- Around line 104-111: The test for LoadExecutorConfig with Clear: true only
verifies the in-memory cleared config and misses the on-disk delete behavior in
SaveExecutorConfig/setOrDelete. Extend the existing executor_config_test case to
save the cleared config back through SaveExecutorConfig, then read the box env
and assert the previously persisted ExecutorWebBaseURLEnv, ExecutorPublishEnv,
and ExecutorDNSEnv keys are removed. Use LoadExecutorConfig, SaveExecutorConfig,
and state.ReadBoxEnv to exercise the full --clear-executor-config path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6ea6349-cb81-4968-b876-9292e0d50a30
📒 Files selected for processing (23)
README.mddocs/docker-architecture.mddocs/tailscale-executor.mddocs/tx9-cli-design.mdinternal/box/box.gointernal/box/box_test.gointernal/box/create.gointernal/box/executor_config.gointernal/box/executor_config_test.gointernal/cli/cmd_create.gointernal/cli/cmd_doctor.gointernal/cli/cmd_doctor_test.gointernal/cli/cmd_gateway.gointernal/cli/cmd_gateway_test.gointernal/cli/cmd_import.gointernal/cli/cmd_list.gointernal/cli/cmd_open.gointernal/cli/cmd_upgrade.gointernal/cli/dispatch.gointernal/cli/executor_config.gointernal/cli/executor_config_test.gointernal/docker/client.gointernal/docker/labels.go
| var publicProbeErr error | ||
| if b.ExecutorWebBaseURL != "" { | ||
| publicProbeErr = probeExecutorPublicURL(b.ExecutorWebBaseURL) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix noctx lint violation: thread ctx through probeExecutorPublicURL.
Static analysis flags client.Get(u.String()) (Line 88) — use Do(*http.Request) with a context-aware request instead. ctx is already available in the enclosing closure; thread it through so the probe is cancelable and lint-clean.
🔧 Proposed fix
- var publicProbeErr error
if b.ExecutorWebBaseURL != "" {
- publicProbeErr = probeExecutorPublicURL(b.ExecutorWebBaseURL)
+ publicProbeErr = probeExecutorPublicURL(ctx, b.ExecutorWebBaseURL)
if publicProbeErr == nil {-func probeExecutorPublicURL(baseURL string) error {
+func probeExecutorPublicURL(ctx context.Context, baseURL string) error {
u, err := url.Parse(baseURL)
if err != nil {
return fmt.Errorf("executor public URL %q is invalid: %w", baseURL, err)
}
u.Path = "/api/health"
u.RawPath = ""
u.RawQuery = ""
u.Fragment = ""
client := &http.Client{Timeout: 10 * time.Second}
- resp, err := client.Get(u.String())
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
+ if err != nil {
+ return fmt.Errorf("executor public health probe %s: %w", u, err)
+ }
+ resp, err := client.Do(req)
if err != nil {
return fmt.Errorf("executor public health probe %s: %w", u, err)
}The test call site in cmd_doctor_test.go needs a matching update:
if err := probeExecutorPublicURL(context.Background(), server.URL); err != nil {Also applies to: 77-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/cmd_doctor.go` around lines 61 - 63, `probeExecutorPublicURL` is
using a non-context-aware HTTP call and triggers the `noctx` lint error; update
it to accept `ctx` and build the request with that context before calling the
client so the probe can be canceled. Propagate the new signature from the
`cmd_doctor` probe call site, and update the matching test call in
`cmd_doctor_test.go` to pass a context (for example `context.Background()`) so
the API stays consistent.
Source: Linters/SAST tools
Macroscope: - gateway status/disable no longer require the host-side token cache; only enable hard-fails without it (disable is the recovery path) - reject web base URLs with a dangling colon (https://host:) or an empty hostname (https://:8443) in normalizeWebBaseURL - create/import ignore a stale persisted ~/.tx9/boxes/<name>.env from a previous same-named box (new IgnoreStored override; upgrade still inherits persisted settings) - upgrade distinguishes created-but-not-started containers from not-recreated ones and says how to recover (labeled containers are removed by a retry; no manual docker cleanup) CodeRabbit: - escape pipes in the tx9 gateway command-table row - OpenURL returns (string, error) instead of swallowing url.Parse failures into an empty string - thread ctx through probeExecutorPublicURL (noctx; cancelable probe) - test that --clear-executor-config deletes persisted TX9_EXECUTOR_* keys on disk, not just in memory Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review-bot findings in 9e59f6c — all 7 inline findings were fixed (none skipped): Macroscope
CodeRabbit
|
| return fmt.Errorf("upgrade %s: %w", name, err) | ||
| } | ||
| fromVersion := b.Version | ||
| executorConfig, err := executorFlags.load(name) |
There was a problem hiding this comment.
🟡 Medium cli/cmd_upgrade.go:53
cmdUpgrade accepts --executor-web-base-url without requiring a fixed --executor-publish address, so after the upgrade Docker assigns a new random host port for the executor while the box advertises the persisted HTTPS origin. Any reverse proxy pointing at the old port becomes stale and the new public URL stops working. Consider rejecting this combination (or falling back to the previously-bound port) so the advertised origin stays reachable.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/cli/cmd_upgrade.go around line 53:
`cmdUpgrade` accepts `--executor-web-base-url` without requiring a fixed `--executor-publish` address, so after the upgrade Docker assigns a new random host port for the executor while the box advertises the persisted HTTPS origin. Any reverse proxy pointing at the old port becomes stale and the new public URL stops working. Consider rejecting this combination (or falling back to the previously-bound port) so the advertised origin stays reachable.
| return fmt.Errorf("import %s: %w", name, err) | ||
| } | ||
| if err := state.WriteBoxEnv(name, map[string]string{"EXECUTOR_MCP_TOKEN": tok}); err != nil { | ||
| if err := box.SaveExecutorConfig(name, tok, executorConfig); err != nil { |
There was a problem hiding this comment.
🟡 Medium cli/cmd_import.go:133
cmdImport calls executorFlags.loadFresh(name) to treat a leftover ~/.tx9/boxes/<name>.env as fresh, but then calls box.SaveExecutorConfig which internally calls state.ReadBoxEnv(name). If that stale file is unreadable (e.g. bad permissions or ownership left by a previous failed create/import), import aborts with a state.ReadBoxEnv error instead of proceeding, so the operator must manually fix or remove the old env file before a new import can succeed. Consider having SaveExecutorConfig handle a missing/unreadable env file by falling back to an empty map, or having cmdImport pass the already-resolved config so the file isn't re-read.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/cli/cmd_import.go around line 133:
`cmdImport` calls `executorFlags.loadFresh(name)` to treat a leftover `~/.tx9/boxes/<name>.env` as fresh, but then calls `box.SaveExecutorConfig` which internally calls `state.ReadBoxEnv(name)`. If that stale file is unreadable (e.g. bad permissions or ownership left by a previous failed create/import), import aborts with a `state.ReadBoxEnv` error instead of proceeding, so the operator must manually fix or remove the old env file before a new import can succeed. Consider having `SaveExecutorConfig` handle a missing/unreadable env file by falling back to an empty map, or having `cmdImport` pass the already-resolved config so the file isn't re-read.
| u.RawQuery = "" | ||
| u.Fragment = "" | ||
|
|
||
| client := &http.Client{Timeout: 10 * time.Second} |
There was a problem hiding this comment.
🟡 Medium cli/cmd_doctor.go:87
probeExecutorPublicURL creates an http.Client with a nil Transport, so Go falls back to http.DefaultTransport and routes the request through any HTTP_PROXY/HTTPS_PROXY environment variable. On hosts with a corporate proxy configured, tx9 doctor sends the health probe through that proxy instead of directly to the local tailnet URL, causing false failures even when the Executor endpoint is healthy. Consider setting a custom Transport that ignores proxy environment variables (e.g. &http.Transport{Proxy: nil}) so the probe always connects directly.
| client := &http.Client{Timeout: 10 * time.Second} | |
| client := &http.Client{ | |
| Timeout: 10 * time.Second, | |
| Transport: &http.Transport{Proxy: nil}, | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/cli/cmd_doctor.go around line 87:
`probeExecutorPublicURL` creates an `http.Client` with a nil `Transport`, so Go falls back to `http.DefaultTransport` and routes the request through any `HTTP_PROXY`/`HTTPS_PROXY` environment variable. On hosts with a corporate proxy configured, `tx9 doctor` sends the health probe through that proxy instead of directly to the local tailnet URL, causing false failures even when the Executor endpoint is healthy. Consider setting a custom `Transport` that ignores proxy environment variables (e.g. `&http.Transport{Proxy: nil}`) so the probe always connects directly.
1 participant
What changed
EXECUTOR_WEB_BASE_URLand Docker DNS into the Executor containerlist,open, anddoctorunderstand and verify the configured HTTPS origintx9 gateway status|enable|disable; enabling rehomes any foreground gateway under TX9 supervision while preserving the single-writer gate--clear-executor-configavailable to remove persisted settingsWhy
Executor previously advertised
http://localhost:4788for OAuth callbacks while TX9 exposed a random raw HTTP host port. Recreating the container could change that port, and the container did not inherit MagicDNS. Hermes setup could also leave a foreground gateway attached to tmux, so closing the pane took it offline.This gives reverse proxies such as Tailscale Serve a stable loopback target and exact HTTPS callback origin while preserving TX9's agent/Executor network isolation.
Validation
PATH=/home/davis/.local/go/bin:$PATH make checkgit diff --checkNote
Add Tailscale HTTPS access support for the Executor with persisted configuration
--executor-web-base-url,--executor-publish,--executor-dns, and--clear-executor-configflags totx9 create,tx9 import, andtx9 upgrade, persisting executor config across upgrades viaexecutor_config.go.tx9 gateway status|enable|disablecommand incmd_gateway.goto manage the supervised Hermes gateway;enablerequires explicit single-writer confirmation.tx9 listandtx9 opennow surface the configured public HTTPS URL (from thetx9.executor-web-base-urlDocker label) instead of the rawhost:port.tx9 doctorprobes the executor's/api/healthendpoint when a public base URL is configured and fails if it is unreachable.create.goanddocker/client.go.tx9 opennow returns an error if the configured public base URL is invalid, whereas previously it always succeeded.Macroscope summarized 9e59f6c.
Greptile Summary
This PR adds Tailscale-friendly HTTPS support for Executor access. The main changes are:
create,import, and box-specificupgrade.list,open, anddoctorbehavior for configured HTTPS origins.tx9 gateway status|enable|disablelifecycle commands.Confidence Score: 4/5
Mostly safe, with one contained bug in
tx9 doctorfor non-loopback publish addresses.The main create/import/upgrade and URL plumbing is well-scoped, but the new configurable publish address is not fully honored by the doctor host probe.
internal/cli/cmd_doctor.goWhat T-Rex did
Important Files Changed
Comments Outside Diff (2)
internal/cli/cmd_doctor.go, line 56 (link)probeHostPortalways checks127.0.0.1, but--executor-publishaccepts any IPv4 bind address. A box recreated with--executor-publish 100.64.1.2:32770publishes the executor only on that host IP, sotx9 doctorfails the local probe even though Docker published the configured endpoint correctly. The probe needs the publishedHostIPfrom Docker, not justHostPort.Artifacts
Repro: focused Go test binding executor endpoint to a non-loopback HostIP and invoking probeHostPort
Repro: verbose go test output showing configured endpoint status 200 OK and doctor probe failure on 127.0.0.1
Prompt To Fix With AI
internal/cli/cmd_doctor.go, line 56 (link)--executor-publishaccepts any IPv4IP:port, butdoctorstill probes127.0.0.1:<port>afterHostPortdrops the publishedHostIP. A valid box published to a non-loopback address, such as a LAN or Tailscale IP, will run correctly whiletx9 doctoralways fails the host probe. Please preserve and probe Docker's published host IP instead of assuming loopback.Artifacts
Repro: focused Go test harness that binds a non-loopback HTTP server, performs a direct request, and invokes probeHostPort
Repro: go test output showing direct HTTP 200 OK on the bound IP and probeHostPort failure against 127.0.0.1
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: address PR #18 review-bot findings" | Re-trigger Greptile