-
Notifications
You must be signed in to change notification settings - Fork 23
Add serve command: combined local server and client in one #101
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.
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
base: master
Are you sure you want to change the base?
Conversation
This adds a new `serve` subcommand that starts a local server and broadcasts the terminal to it in a single command, eliminating the need to run server and client separately for local sharing. Options: --host/-H: Bind address (default: 127.0.0.1) --port/-p: Listen port (default: 5000) --stdin: Read from stdin instead of spawning a shell The server automatically shuts down when the session ends. https://claude.ai/code/session_01HsviQhQjYdFSDSU7mpZmXw
When running `shellshare serve`, GET / now serves the room viewer page instead of the home page. This means visitors can go to http://localhost:5000 and see the terminal directly. Implementation: - Add `ServerConfig` struct with optional `serve_room` field - In serve mode, inject `window.SHELLSHARE_ROOM` into room.html so the JS knows which room to join regardless of URL path - Display clean base URL (http://localhost:5000) in output - Regular `shellshare server` is unaffected (serve_room: None) https://claude.ai/code/session_01HsviQhQjYdFSDSU7mpZmXw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a new shellshare serve CLI subcommand that runs a local server and immediately connects a local client to it, enabling “one command” terminal sharing (with / showing the terminal viewer directly).
Changes:
- Introduces
shellshare servewith--host,--port, and--stdinoptions to run a combined server+client flow. - Extends server routing to optionally serve a specific room viewer at
/by injecting a room override intoroom.html. - Adds a comprehensive E2E test suite for serve mode (stdin + PTY behaviors).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/room.html | Allows room selection via an injected window.SHELLSHARE_ROOM override. |
| src/server/mod.rs | Adds ServerConfig and serve-mode / routing that serves the room viewer directly. |
| src/main.rs | Wires the new Serve subcommand and updates server startup to use ServerConfig. |
| src/cli/serve.rs | Implements combined server+client execution, readiness polling, URL printing, and cleanup. |
| src/cli/mod.rs | Exposes stream_stdin for reuse by serve mode. |
| e2e/test_cli_serve.py | Adds E2E coverage for serve mode behavior and lifecycle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Inject the room override before the main script | ||
| let override_script = | ||
| format!("<script>window.SHELLSHARE_ROOM=\"/r/{room}\";</script>"); | ||
| let html = html.replacen("<script>", &format!("{override_script}\n <script>"), 1); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serve_room_index_handler injects room directly into a JS string (window.SHELLSHARE_ROOM="/r/{room}") without escaping. If ServerConfig.serve_room ever comes from user input (or contains quotes/</script>), this can become an XSS vector. Consider normalizing the room name (e.g., stripping leading /r/) and escaping via JSON string encoding before embedding into HTML.
| // Inject the room override before the main script | ||
| let override_script = | ||
| format!("<script>window.SHELLSHARE_ROOM=\"/r/{room}\";</script>"); | ||
| let html = html.replacen("<script>", &format!("{override_script}\n <script>"), 1); | ||
|
|
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Room injection currently relies on html.replacen("<script>", ...) to find the first inline <script> tag. This is brittle (breaks if the template changes to <script type=...> or rearranges scripts) and silently serves the wrong page without the override. Consider adding an explicit placeholder in room.html for the override (or using a minimal templating step) instead of string-search replacement.
| // Inject the room override before the main script | |
| let override_script = | |
| format!("<script>window.SHELLSHARE_ROOM=\"/r/{room}\";</script>"); | |
| let html = html.replacen("<script>", &format!("{override_script}\n <script>"), 1); | |
| // Inject the room override script using an explicit placeholder if present, | |
| // otherwise fall back to inserting before the first <script> tag, then </body>, | |
| // and finally at the end of the document. | |
| let override_script = | |
| format!("<script>window.SHELLSHARE_ROOM=\"/r/{room}\";</script>"); | |
| let html = { | |
| const PLACEHOLDER: &str = "{{ROOM_OVERRIDE_SCRIPT}}"; | |
| if html.contains(PLACEHOLDER) { | |
| // Preferred: template provides an explicit placeholder. | |
| html.replacen(PLACEHOLDER, &override_script, 1) | |
| } else if let Some(script_pos) = html.find("<script") { | |
| // Fallback: insert before the first <script...> tag (handles attributes). | |
| let mut result = | |
| String::with_capacity(html.len() + override_script.len() + 1); | |
| result.push_str(&html[..script_pos]); | |
| result.push_str(&override_script); | |
| result.push('\n'); | |
| result.push_str(&html[script_pos..]); | |
| result | |
| } else if let Some(body_pos) = html.rfind("</body>") { | |
| // Fallback: insert before closing </body> if there are no script tags. | |
| let mut result = | |
| String::with_capacity(html.len() + override_script.len() + 1); | |
| result.push_str(&html[..body_pos]); | |
| result.push_str(&override_script); | |
| result.push('\n'); | |
| result.push_str(&html[body_pos..]); | |
| result | |
| } else { | |
| // Last resort: append at the end. | |
| let mut result = | |
| String::with_capacity(html.len() + override_script.len() + 1); | |
| result.push_str(&html); | |
| result.push('\n'); | |
| result.push_str(&override_script); | |
| result | |
| } | |
| }; |
| let server_config = crate::server::ServerConfig { | ||
| host: args.host.clone(), | ||
| port: args.port, | ||
| cleanup_interval_secs: 3600, | ||
| room_ttl_secs: 21600, | ||
| serve_room: Some(ROOM_NAME.to_string()), | ||
| }; |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup_interval_secs / room_ttl_secs are hard-coded (3600/21600), duplicating the server subcommand defaults. This can drift if defaults change. Consider reusing shared constants (or wiring these through CLI args) so behavior stays consistent across commands.
| let server_config = crate::server::ServerConfig { | |
| host: args.host.clone(), | |
| port: args.port, | |
| cleanup_interval_secs: 3600, | |
| room_ttl_secs: 21600, | |
| serve_room: Some(ROOM_NAME.to_string()), | |
| }; | |
| let mut server_config = crate::server::ServerConfig::default(); | |
| server_config.host = args.host.clone(); | |
| server_config.port = args.port; | |
| server_config.serve_room = Some(ROOM_NAME.to_string()); |
| let display_host = display_host(&args.host); | ||
| let base_url = format!("http://{display_host}:{}", args.port); | ||
|
|
||
| // Create HTTP client pointing to the local server | ||
| let server_url = format!("http://{connect_host}:{}", args.port); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base_url / server_url are built with format!("http://{host}:{port}"), which produces invalid URLs for IPv6 addresses (needs brackets) and mis-handles hosts containing :. Consider validating --host to an IpAddr/SocketAddr, or using a URL builder that formats IPv6 hosts correctly.
| stop_serve(proc) | ||
|
|
||
| # After exit, a new serve on the same port should work | ||
| # (room was cleaned up, not locked by stale password) | ||
| port2 = get_free_port() |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test stops the process via stop_serve(proc) (which sends terminate() by default), so it may not exercise the clean-exit/DELETE cleanup path it claims to test. Also, the comment says “same port”, but the follow-up run uses port2. Consider either exiting cleanly and asserting behavior that depends on DELETE/room cleanup, or renaming/updating the test to match what it actually verifies.
| fn test_wait_for_server_fails_on_closed_port() { | ||
| // Port 1 is almost certainly not listening | ||
| let result = wait_for_server("127.0.0.1", 1); | ||
| assert!(result.is_err()); | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_wait_for_server_fails_on_closed_port assumes port 1 is not listening, which can be flaky on some environments. Consider selecting a guaranteed-closed port by binding an ephemeral port, reading it, closing it, and then asserting connect fails (or otherwise ensuring the chosen port is unused).
| // Shut down the runtime without blocking | ||
| runtime.shutdown_background(); | ||
|
|
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime.shutdown_background() is only called on the normal success path. If any ? early-returns after the server task is spawned, dropping the Tokio runtime can block while shutting down the still-running server task. Consider using a Drop/scope guard so shutdown happens on all exit paths (and optionally do best-effort room cleanup on errors).
Summary
Adds a new
shellshare servecommand that starts a local server and immediately connects to it, allowing users to share their terminal with a single command. This provides a simpler alternative to running separate server and client processes.Key Changes
New
servesubcommand (src/cli/serve.rs): Implements the combined server+client functionality--stdin: Reads from stdin (prints sharing URL to stderr)CLI argument parsing (
src/main.rs): AddedServecommand variant with options:--host(default:127.0.0.1): Bind address for the server--port(default:5000): Port to listen on--stdin: Enable stdin mode instead of PTY modeModule exports (
src/cli/mod.rs): Madestream_stdinpublic to allow reuse by serve modeComprehensive E2E tests (
e2e/test_cli_serve.py): 373 lines of test coverage including:Implementation Details
"terminal") since only one room is needed per serve session0.0.0.0to127.0.0.1for connection attempts (platform compatibility)localhostin user-facing URLs for better UXhttps://claude.ai/code/session_01HsviQhQjYdFSDSU7mpZmXw