Skip to content

fix(cli): respect SURFPOOL_STUDIO_HOST environment variable for server binding #699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Amit5601 wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −1
Open

fix(cli): respect SURFPOOL_STUDIO_HOST environment variable for server binding #699

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion crates/cli/src/http/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,25 @@ pub async fn start_studio_and_scenario_server(
let template_registry_wrapped = Data::new(RwLock::new(TemplateRegistry::new()));
let loaded_scenarios = Data::new(RwLock::new(LoadedScenarios::new()));

let default_port = network_binding.split(':').last().unwrap_or("18488");

@greptile-apps greptile-apps Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 unwrap_or("18488") is unreachable — split on a non-empty string always yields at least one element

str::split on a non-empty network_binding string always returns at least one element, so .last() always returns Some(_), meaning .unwrap_or("18488") can never activate. If network_binding carries no colon (e.g. a bare hostname), default_port becomes the whole network_binding string and the formatted address will be malformed. Using rsplit_once makes the intent explicit and provides a real fallback.

Suggested change
let default_port = network_binding.split(':').last().unwrap_or("18488");
let default_port = network_binding
.rsplit_once(':')
.map(|(_, port)| port)
.unwrap_or("18488");

let final_bind_addr = std::env::var("SURFPOOL_STUDIO_HOST")
.map(|host| {
if host.contains(']') {
if host.rfind(':') > host.rfind(']') {
host
} else {
format!("{}:{}", host, default_port)
}
} else {
match host.matches(':').count() {
0 => format!("{}:{}", host, default_port),
1 => host,
_ => format!("[{}]:{}", host, default_port),
}
Comment on lines +64 to +68

@greptile-apps greptile-apps Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Verbatim passthrough for the count == 1 case doesn't guard against a missing port

When SURFPOOL_STUDIO_HOST contains exactly one colon (e.g. "myhost:" or ":") the value is returned as-is. actix-web will then fail at bind() with a generic "invalid address" error that gives no hint the env var is to blame. Verifying that the part after the colon is non-empty would surface the mis-configuration before the bind attempt.

Suggested change
match host.matches(':').count() {
0 => format!("{}:{}", host, default_port),
1 => host,
_ => format!("[{}]:{}", host, default_port),
}
match host.matches(':').count() {
0 => format!("{}:{}", host, default_port),
1 => {
if host.rsplit_once(':').map_or(false, |(_, p)| !p.is_empty()) {
host
} else {
format!("{}:{}", host.trim_end_matches(':'), default_port)
}
}
_ => format!("[{}]:{}", host, default_port),
}

}
})
.unwrap_or(network_binding);
Comment on lines +55 to +71

@greptile-apps greptile-apps Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No log when env var overrides the bind address

When SURFPOOL_STUDIO_HOST is set, the resolved bind address is silently substituted for network_binding with no log output. In Docker/containerised environments — exactly the use-case this PR targets — a missing or mis-typed env var will leave operators with no indication of which address the server actually tried to bind on. Adding an info!("Binding studio server to {} (from SURFPOOL_STUDIO_HOST)", final_bind_addr) line before the HttpServer::new(...) call would make this diagnosable without a debugger.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


let server = HttpServer::new(move || {
let mut app = App::new()
.app_data(config_wrapped.clone())
Expand Down Expand Up @@ -81,7 +100,7 @@ pub async fn start_studio_and_scenario_server(
app
})
.workers(5)
.bind(network_binding)?
.bind(final_bind_addr)?
.run();
let handle = server.handle();
tokio::spawn(server);
Expand Down