feat(cli): add --with-server flag to CLI mode#154
Open
puwun wants to merge 2 commits intoqualifire-dev:mainfrom
Open
feat(cli): add --with-server flag to CLI mode#154puwun wants to merge 2 commits intoqualifire-dev:mainfrom
puwun wants to merge 2 commits intoqualifire-dev:mainfrom
Conversation
Contributor
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Entrypoint as CLI/TUI
participant ServerStarter
participant BackgroundServer
User->>Entrypoint: run command with --with-server
Entrypoint->>ServerStarter: request start background server (host, port)
ServerStarter->>BackgroundServer: spawn process
BackgroundServer-->>ServerStarter: process started (async)
ServerStarter->>ServerStarter: normalize host (get_host_for_url) and wait for readiness
alt ready
ServerStarter-->>Entrypoint: return server URL
Entrypoint->>Entrypoint: execute CLI/TUI logic (uses server URL)
Entrypoint->>ServerStarter: request shutdown
ServerStarter->>BackgroundServer: terminate & join
else not ready
ServerStarter->>BackgroundServer: terminate & join (5s timeout)
ServerStarter-->>Entrypoint: raise startup error
Entrypoint-->>User: exit with error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rogue/__main__.py`:
- Around line 273-288: The code calls asyncio.run(run_cli(args)) without
catching exceptions, which can leave exit_code undefined and cause
UnboundLocalError later; wrap the asyncio.run(run_cli(args)) call in a
try/except that catches Exception as e, logs the failure via logger.error
(including e), sets exit_code to a non-zero value (e.g., 1), and then let the
finally block terminate/join server_process as before so sys.exit(exit_code)
always has a value; reference run_cli, asyncio.run, exit_code, server_process
and logger in your change.
puwun
force-pushed
the
feature/cli-with-server
branch
from
dbb4f98 to
14e7614
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@rogue/__main__.py`:
- Around line 273-281: The call to run_server(..., background=True) inside the
args.with_server branch can raise exceptions that bypass the existing
falsy-return check; wrap the run_server call in a try/except that catches
Exception, log the error with logger.error including the exception information,
and call sys.exit(1) on failure (same behavior as the falsy-return case). Ensure
you still assign server_process on success and preserve the existing check for
falsy returns (i.e., if not server_process: logger.error(...); sys.exit(1)) so
both exceptions and false returns are handled.
- Around line 274-285: After starting the background server with
run_server(args, background=True) ensure args.rogue_server_url points at the
actual server address instead of the hardcoded default: update run_server to
return the real URL (or host/port) along with the process (e.g., return
(process, url) or set process.rogue_server_url/process.host/process.port), then
after server_process is assigned set args.rogue_server_url = returned_url (or
build "http://{host}:{port}" from process.host/process.port) before calling
asyncio.run(run_cli(args)); reference run_server, server_process, args and
args.rogue_server_url when making the change.
14e7614 to
e261213
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@rogue/__main__.py`:
- Around line 274-287: The code sets args.rogue_server_url directly from
args.host which produces a non-routable URL when the server binds to all
interfaces (e.g., "0.0.0.0" or "::"); after run_server and verifying
server_process, detect if args.host is an all-interfaces address and if so
substitute a loopback (e.g., "127.0.0.1" or "localhost") before building
args.rogue_server_url, otherwise use args.host as-is; update the assignment of
args.rogue_server_url accordingly (references: args.rogue_server_url, args.host,
args.port, run_server, server_process).
- Around line 288-293: The try/except around asyncio.run(run_cli(args)) can let
KeyboardInterrupt (and other BaseException subclasses) bypass the Exception
handler and leave exit_code undefined; update the block around asyncio.run in
__main__.py to also catch KeyboardInterrupt (or BaseException) and set exit_code
to a suitable value (e.g., 1), and log via logger.error or logger.warning
including the interrupt info so that sys.exit(exit_code) always has a defined
value; ensure references include asyncio.run, run_cli, exit_code, logger, and
sys.exit(exit_code).
01578a7 to
efcb347
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rogue/__main__.py`:
- Around line 273-305: The background server spawned by run_server
(rogue/run_server.py) can be orphaned when readiness checks fail and an
exception is raised before the process handle is returned; update run_server to
always clean up the spawned subprocess on readiness failure (terminate/kill and
join) before raising, or change the error path to raise an exception that
includes the process handle so callers (e.g., __main__.py's server_process
variable) can reliably terminate it; modify the failure branch near the
readiness-check code (where the exception is raised around line 148) to perform
process.terminate()/process.kill() and process.join() (or attach the process to
the exception) so __main__.py's try/except can never leave a running orphaned
process.
puwun
force-pushed
the
feature/cli-with-server
branch
from
efcb347 to
6155bcd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rogue/__main__.py (1)
313-321: TUI mode is missing exception handling forrun_servercall.Unlike the CLI mode (lines 275-282), the TUI mode's
run_servercall isn't wrapped in atry/except. Sincerun_server(..., background=True)can raise anException("Server failed to start")when readiness checks fail, an unhandled exception will propagate with a traceback instead of a clean error message and exit.🔧 Suggested fix
server_process = None if args.with_server: - server_process = run_server( - args, - background=True, - log_file=log_file_path, - ) - if not server_process: - logger.error("Failed to start rogue server. Exiting.") + try: + server_process = run_server( + args, + background=True, + log_file=log_file_path, + ) + except Exception as e: + logger.error(f"Failed to start rogue server: {e}") + sys.exit(1) + if not server_process: + logger.error("Failed to start rogue server. Exiting.") sys.exit(1)
🤖 Fix all issues with AI agents
In `@rogue/__main__.py`:
- Around line 286-291: The URL construction using client_host and
args.rogue_server_url doesn't bracket IPv6 addresses other than "::", producing
invalid URLs like http://::1:8000; update the logic that sets client_host (and
thus args.rogue_server_url) to detect when args.host is an IPv6 address (use the
ipaddress module or reuse the helper from wait_until_server_ready in
run_server.py), and if it's IPv6 wrap the address in [ ] before interpolating
into f"http://{client_host}:{args.port}"; add or move the ipaddress import to
the top of the file if needed.
- Extract get_host_for_url utility - Fix orphan server process on startup failure - Auto-configure MCP transport for examples
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
--with-serverflag to the CLI mode, allowing users to start the rogue server alongside the CLI in the same commandType of Change
Changes Made
--with-serverargument to the CLI parser inrogue/__main__.pyRelated Issues/PRs