fix(macos): improve tailscale gateway discovery#40167
Conversation
openclaw-barnacle
bot
added
app: macos
Greptile SummaryThis PR fixes three independent macOS-only bugs in Tailscale gateway discovery: (1) Tailscale Serve probing was being suppressed as soon as any DNS-SD remote gateway appeared; (2) selecting a Tailscale Serve or resolved Key changes:
Confidence Score: 4/5
Last reviewed commit: f4db070 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func shouldContinueTailscaleServeDiscovery( | ||
| currentGateways _: [DiscoveredGateway], | ||
| tailscaleServeGateways: [DiscoveredGateway]) -> Bool | ||
| { | ||
| // Tailscale Serve is a parallel discovery source. DNS-SD results should not suppress the | ||
| // probe, otherwise Serve-only gateways disappear as soon as any other remote gateway is found. | ||
| tailscaleServeGateways.isEmpty | ||
| } |
There was a problem hiding this comment.
Unused parameter signals future API ambiguity
currentGateways is labeled _ and entirely ignored in the implementation — only tailscaleServeGateways.isEmpty is evaluated. While the in-line comment explains the intent clearly, keeping a labeled-but-discarded parameter in a static helper makes the call-site misleading: callers pass self.gateways expecting it to influence the result, but it never does.
If there are no near-term plans to use currentGateways, consider removing it from the signature and updating the call-site accordingly:
| static func shouldContinueTailscaleServeDiscovery( | |
| currentGateways _: [DiscoveredGateway], | |
| tailscaleServeGateways: [DiscoveredGateway]) -> Bool | |
| { | |
| // Tailscale Serve is a parallel discovery source. DNS-SD results should not suppress the | |
| // probe, otherwise Serve-only gateways disappear as soon as any other remote gateway is found. | |
| tailscaleServeGateways.isEmpty | |
| } | |
| static func shouldContinueTailscaleServeDiscovery( | |
| tailscaleServeGateways: [DiscoveredGateway]) -> Bool | |
| { | |
| // Tailscale Serve is a parallel discovery source. DNS-SD results should not suppress the | |
| // probe, otherwise Serve-only gateways disappear as soon as any other remote gateway is found. | |
| tailscaleServeGateways.isEmpty | |
| } |
And update the call-site in scheduleTailscaleServeFallback to drop the unused argument. If the parameter is intentionally reserved for a future condition, a comment noting that intent would prevent it from being pruned by the next contributor.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/macos/Sources/OpenClawDiscovery/GatewayDiscoveryModel.swift
Line: 365-372
Comment:
**Unused parameter signals future API ambiguity**
`currentGateways` is labeled `_` and entirely ignored in the implementation — only `tailscaleServeGateways.isEmpty` is evaluated. While the in-line comment explains the intent clearly, keeping a labeled-but-discarded parameter in a `static` helper makes the call-site misleading: callers pass `self.gateways` expecting it to influence the result, but it never does.
If there are no near-term plans to use `currentGateways`, consider removing it from the signature and updating the call-site accordingly:
```suggestion
static func shouldContinueTailscaleServeDiscovery(
tailscaleServeGateways: [DiscoveredGateway]) -> Bool
{
// Tailscale Serve is a parallel discovery source. DNS-SD results should not suppress the
// probe, otherwise Serve-only gateways disappear as soon as any other remote gateway is found.
tailscaleServeGateways.isEmpty
}
```
And update the call-site in `scheduleTailscaleServeFallback` to drop the unused argument. If the parameter is intentionally reserved for a future condition, a comment noting that intent would prevent it from being pruned by the next contributor.
How can I resolve this? If you propose a fix, please make it concise.| state.remoteUrl = GatewayDiscoveryHelpers.directUrl(for: gateway) ?? "" | ||
| state.remoteTarget = GatewayDiscoveryHelpers.sshTarget(for: gateway) ?? "" |
There was a problem hiding this comment.
Both state fields are now always written on every gateway selection
Previously remoteUrl was only written in .direct mode and remoteTarget only in .ssh mode, so whichever field wasn't active was left holding its previous value. The new code always sets both, which is logically cleaner (a later transport switch will find both values populated).
One edge to be aware of: if serviceHost is nil or empty (e.g. a Bonjour service that hasn't resolved yet), both directUrl(for:) and sshTarget(for:) return nil, so both fields are overwritten with "". Under the old code only the active field would have been blanked. This is unlikely to surface in practice — the gateway list typically only shows entries with resolved hosts — but if a half-resolved gateway could ever be passed to applyRemoteSelection, the caller's previous direct URL would now also be lost. Worth a quick sanity-check in the calling code that only fully-resolved gateways are passed here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/macos/Sources/OpenClaw/GatewayDiscoverySelectionSupport.swift
Line: 16-17
Comment:
**Both state fields are now always written on every gateway selection**
Previously `remoteUrl` was only written in `.direct` mode and `remoteTarget` only in `.ssh` mode, so whichever field wasn't active was left holding its previous value. The new code always sets both, which is logically cleaner (a later transport switch will find both values populated).
One edge to be aware of: if `serviceHost` is `nil` or empty (e.g. a Bonjour service that hasn't resolved yet), both `directUrl(for:)` and `sshTarget(for:)` return `nil`, so both fields are overwritten with `""`. Under the old code only the active field would have been blanked. This is unlikely to surface in practice — the gateway list typically only shows entries with resolved hosts — but if a half-resolved gateway could ever be passed to `applyRemoteSelection`, the caller's previous direct URL would now also be lost. Worth a quick sanity-check in the calling code that only fully-resolved gateways are passed here.
How can I resolve this? If you propose a fix, please make it concise.
ngutman
force-pushed
the
fix/macos-tailscale-gateway-discovery
branch
2 times, most recently
from
9b793f0 to
bb923c6
Compare
ngutman
force-pushed
the
fix/macos-tailscale-gateway-discovery
branch
from
bb923c6 to
5d14656
Compare
|
Landed via squash after sanitizing the macOS test tailnet hostname.
Thanks @ngutman! |
Sanitized test tailnet hostnames and re-ran the targeted macOS gateway discovery test suite before merge.
Sanitized test tailnet hostnames and re-ran the targeted macOS gateway discovery test suite before merge.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
tailscale status --jsonwhenTERMwas missing..ts.netgateways, and setTERM=dumbfor the Tailscale subprocess when needed.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
.ts.netendpoints now default to direct transport when selected.Security Impact (required)
No)No)Yes)Yes)No)Yes, explain risk + mitigation:This changes when existing Tailscale Serve probes run and how the existing
tailscale status --jsonsubprocess is launched from the macOS app. Routing still uses resolved service endpoints rather than TXT hints, and the subprocess change is limited to supplyingTERM=dumbwhen the GUI environment omits it.Repro + Verification
Environment
Steps
Expected
TERMcan still query Tailscale status.Actual
Evidence
Attach at least one:
Targeted test command run locally:
Result: 22 tests passed across 3 suites.
Human Verification (required)
What you personally verified (not just CI), and how:
TERMis preserved.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None..ts.netgateways.