-
-
Notifications
You must be signed in to change notification settings - Fork 285
add --host option to restrict listening interface #157
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
WalkthroughThis change introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (2)
154-167: Clarify default host wording in options tableThe new
--hostrow looks good and matches the implementation/CLI. Consider rewording the default fromall (::)to something likeall interfaces (system default)so users don’t confuse it with a literal::hostname value.
168-185:--hostexamples are clear and helpfulThe IPv4/IPv6 localhost examples and default behavior are well illustrated and align with the new CLI option. You might optionally add a short example using the
-halias for completeness, but the section is already solid as-is.src/start.ts (1)
144-149: Check for potential-h(help) vs-h(host) alias conflictDefining
hostwith alias-his intuitive, but many CLIs reserve-hfor help. citty’s docs don’t clearly specify whether-his used/reserved by your current entrypoint, so there’s some risk of confusing or overriding help behavior.Please run your top‑level CLI with
-hand--help(e.g.,copilot-api -h,copilot-api --help, andcopilot-api start -h) to confirm:
- Help output is still accessible in the expected way.
-hcorrectly maps to--hostwhere intended.If there is a collision, consider switching the short flag to something like
-Hor dropping the short alias forhost.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)src/start.ts(5 hunks)
🧰 Additional context used
🪛 ESLint
src/start.ts
[error] 69-69: 'hostForUrl' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 69-69: Replace 'localhost'; with "localhost"
(prettier/prettier)
🔇 Additional comments (3)
src/start.ts (3)
17-29: RunServerOptions.host field wiring is appropriateAdding an optional
host?: stringhere is a clean way to thread the CLI--hostflag through to the server; the type and optionality both look correct.
115-123: Hostname forwarding intoserve()matches srvx semanticsPassing
hostname: options.host || undefinedcorrectly lets srvx bind to the specified interface, and falls back to “all interfaces” when--hostis omitted, which aligns with the srvx docs for thehostnameoption (undefined => all network interfaces). (srvx.h3.dev)If you upgrade srvx later, please double‑check that this behavior remains the same in that version’s documentation.
203-215: Threadingargs.hostinto runServer is correctWiring
host: args.hostintorunServercleanly connects the new CLI option to the server binding logic, and the optional nature of the flag is preserved end to end.
| const serverUrl = `http://localhost:${options.port}` | ||
| const hostForUrl = options.host || 'localhost'; | ||
|
|
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.
Fix unused hostForUrl and align serverUrl with the selected host (including IPv6)
hostForUrl is currently unused, causing the ESLint error, and the usage URL still hardcodes localhost, ignoring options.host. You can both fix lint and make the Usage Viewer / Claude Code URLs reflect the actual bind host (with basic IPv6 handling) by updating this block:
- const serverUrl = `http://localhost:${options.port}`
- const hostForUrl = options.host || 'localhost';
+ const hostForUrl =
+ options.host &&
+ options.host.includes(":") &&
+ !options.host.startsWith("[")
+ ? `[${options.host}]`
+ : options.host || "localhost";
+ const serverUrl = `http://${hostForUrl}:${options.port}`;This:
- Removes the unused-variable error and fixes the Prettier single‑quote warning.
- Makes the printed Usage Viewer URL and Claude Code env script consistent with a user‑supplied host.
- Produces valid URLs for IPv6 literals (e.g.,
::1→http://[::1]:4141).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const serverUrl = `http://localhost:${options.port}` | |
| const hostForUrl = options.host || 'localhost'; | |
| const hostForUrl = | |
| options.host && | |
| options.host.includes(":") && | |
| !options.host.startsWith("[") | |
| ? `[${options.host}]` | |
| : options.host || "localhost"; | |
| const serverUrl = `http://${hostForUrl}:${options.port}`; |
🧰 Tools
🪛 ESLint
[error] 69-69: 'hostForUrl' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 69-69: Replace 'localhost'; with "localhost"
(prettier/prettier)
🤖 Prompt for AI Agents
In src/start.ts around lines 68 to 70, replace the hardcoded `localhost` with
the `hostForUrl` value and eliminate the unused variable by building `serverUrl`
from `hostForUrl`; specifically, set hostForUrl = options.host || "localhost"
and then construct serverUrl = `http://<hostForUrl>:${options.port}` but ensure
IPv6 literals are wrapped in brackets when they contain a colon (e.g., if
hostForUrl.includes(':') and not already bracketed, use `[${hostForUrl}]`), and
use double quotes for string literals to satisfy Prettier.
Adds a
--host(alias-h) option to thestartsubcommand so users can bind the Copilot API server to a specific interface/IP (e.g.127.0.0.1or::1) instead of all interfaces.Changes
RunServerOptionswithhost?: string.hostnametoserve(...); fallback (omitting--host) preserves previous behavior.--host,-h).Rationale
Binding to all interfaces (
0.0.0.0/::) can unintentionally expose the API on shared or cloud hosts. Allowing explicit loopback binding improves security.Backward Compatibility
No change for existing users who don’t supply
--host. All interfaces remain default.Testing
bun run buildbun run dist/main.js start --host 127.0.0.1→ confirmed127.0.0.1:4141bun run dist/main.js start --host ::1→ confirmed IPv6 loopback--host→ confirmed:::4141Summary by CodeRabbit
New Features
--host(alias-h) option to the start command, enabling users to specify which host or interface the server listens on. Supports IPv4 localhost (127.0.0.1), IPv6 localhost (::1), and all interfaces. Updated documentation with configuration examples.Documentation
--hostoption with common configurations.✏️ Tip: You can customize this high-level summary in your review settings.