feat(cli): add server subcommand for eval-hub-server lifecycle management#149
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a shared ChangesShared process lifecycle helpers and MCP refactor
Minor adapter cleanups
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ment Add `evalhub server run/start/stop/status` commands to manage the local eval-hub-server binary. Server config is managed through the unified `config set/get/unset` commands via the `server_config_file` key — the file is validated and copied into ~/.config/evalhub/server/<profile>/ on set, deleted on unset. Add `--unfold` flag to `config get` for printing file-based config contents. Co-Authored-By: Claude <noreply@anthropic.com>
… module Deduplicate process management code between server_cmd.py and mcp_cmd.py by extracting shared helpers (find_binary, live_pid, graceful_stop, spawn_background) into cli/_process.py. Also combine _read_server_port and _is_tls_enabled into a single _read_server_config function that parses the YAML once, and inline the trivial _server_scheme helper. Co-Authored-By: Claude <noreply@anthropic.com>
gnaulak-redhat
force-pushed
the
feat-evalhub-server
branch
from
79e98b2 to
cbf4d26
Compare
…ver/mcp commands Extract run_foreground, require_not_running, and stop_daemon helpers into _process.py to eliminate duplicated daemon lifecycle code between server_cmd and mcp_cmd. Remove --config-dir option from server run/start in favor of server_config_file as the single config source. Fix minor simplifications: unnecessary deferred click import, verbose TLS detection, redundant int() cast on resp.status. Co-Authored-By: Claude <noreply@anthropic.com>
gnaulak-redhat
force-pushed
the
feat-evalhub-server
branch
from
353ad12 to
accf25c
Compare
![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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/evalhub/cli/_process.py`:
- Around line 45-50: The live_pid() helper only validates that a PID is
currently running, which is insufficient because PID reuse can make
require_not_running() and stop_daemon() act on the wrong process. Update
live_pid() in _process.py to verify daemon identity with a stronger check than
PID alone, using a stable identifier stored with the pid file and validated
against the running process before returning it. Make sure the same identity
check is used by require_not_running() and stop_daemon() so they only treat the
original daemon as live.
- Around line 53-59: The env override handling in find_binary should validate
that the path from env_var actually exists before returning it, since currently
it returns the raw string and lets the later subprocess call fail with a
traceback. Update find_binary to check the environment-supplied path with an
existence/file check, and if it is missing fall back to the normal lookup or
raise the same ClickException path used for other missing binaries.
- Around line 35-42: read_pid() currently accepts any integer from the pid file,
which allows 0 or negative values to flow into stop_daemon() and then os.kill().
Update read_pid() in _process.py to validate the parsed PID and return None
unless it is strictly positive, keeping the existing handling for missing files
and parse/read errors. Use the read_pid() helper as the single place to reject
invalid pid values so stop_daemon() only ever receives a safe positive process
ID.
In `@src/evalhub/cli/config.py`:
- Around line 188-189: The stored server config copy in the config save path
currently uses shutil.copy2(), which preserves permissive source permissions.
Update the copy logic in the config-saving flow around save_config() so the
destination under ~/.config/evalhub/server is explicitly chmodded to 0600 after
copying, matching the private-permissions behavior already used elsewhere.
Ensure the fix is applied where the copied file is written so the saved config
is never left world-readable.
In `@src/evalhub/cli/server_cmd.py`:
- Around line 143-156: The startup failure path in server_cmd.py leaves a live
daemon running when _wait_for_healthy() fails but proc.poll() is still None,
because PID_FILE is never written and the process is not cleaned up. Update the
failure handling around spawn_background(), _wait_for_healthy(), and the proc
check to explicitly terminate/kill the spawned server and wait for it before
raising the ClickException, using LOG_FILE for diagnostics if needed, so evalhub
server stop does not depend on a missing PID file.
In `@tests/unit/test_cli_server.py`:
- Around line 10-13: This test module under tests/unit/ is missing the required
unit-test marker, so it won’t be included in marker-based runs. Add a
module-level pytest marker in test_cli_server so all tests in this file are
treated as unit tests, following the repository convention for tests/**/*.py.
Use the existing imports and module scope in test_cli_server to place the marker
alongside the current test setup.
- Around line 29-31: The fixture that restores EVALHUB_TOKEN in
test_cli_server.py leaves the variable set when it was originally absent, which
leaks state into later tests. Update the fixture cleanup logic around the
saved_token handling so that when saved_token is None it removes EVALHUB_TOKEN
from os.environ, and when a value was originally present it restores that value.
Use the existing fixture and saved_token variable to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f029fa0f-60e5-4aea-ba4e-a0148613bf02
📒 Files selected for processing (9)
src/evalhub/adapter/auth.pysrc/evalhub/adapter/mlflow.pysrc/evalhub/cli/_process.pysrc/evalhub/cli/config.pysrc/evalhub/cli/main.pysrc/evalhub/cli/mcp_cmd.pysrc/evalhub/cli/server_cmd.pytests/unit/test_cli_mcp.pytests/unit/test_cli_server.py
- Reject non-positive PIDs in read_pid to avoid invalid process lookups - Validate that the binary path from env var actually exists before use - Restrict permissions on stored config files to owner-only (0600) - Terminate orphaned server process on health-check timeout instead of leaving it running - Add missing EVALHUB_TOKEN cleanup in test fixture teardown to prevent env leakage between tests - Add pytest unit marker to server CLI tests Co-Authored-By: Claude <noreply@anthropic.com>
gnaulak-redhat
added
the
ready-for-review
ruivieira
requested changes
Jun 30, 2026
ruivieira
left a comment
ruivieira
left a comment
Member
There was a problem hiding this comment.
@gnaulak-redhat LGTM, thanks! Left a couple of suggestions.
| tls = bool(cert and key) | ||
| return port, tls | ||
| except (yaml.YAMLError, TypeError, ValueError, AttributeError): | ||
| return _DEFAULT_PORT, False |
Member
There was a problem hiding this comment.
Should we add something like
click.echo(f"Warning: could not parse server config ({exc}), using defaults.", err=True)
to give user feedback when there's an exception parsing YAML?
Collaborator
Author
There was a problem hiding this comment.
good point. I updated to "fail" instead of using defaults. Thanks
…g defaults When config.yaml exists but contains invalid values (e.g. non-numeric port), raise a ClickException with the parse error instead of falling back to default settings, which could cause the server to start with unintended configuration. Co-Authored-By: Claude <noreply@anthropic.com>
ruivieira
approved these changes
Jul 1, 2026
Member
|
@coderabbitai please open an issue on https://github.com/eval-hub/eval-hub.github.io to document the changes on this PR. |
4 tasks
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="eval-hub/eval-hub.github.io"
body=$(cat <<'EOF'
## Summary
Document the new `evalhub server` CLI command group and related server configuration management introduced in eval-hub/eval-hub-sdk#149.
## Background
PR eval-hub/eval-hub-sdk#149 adds:
- An `evalhub server` command group for managing the `eval-hub-server` lifecycle:
- `evalhub server run` — run the server in the foreground.
- `evalhub server start` — start the server as a background daemon (writes a PID file and log file, waits for the health endpoint before reporting success).
- `evalhub server stop` — stop the background daemon (graceful shutdown with a timeout, then force-kill).
- `evalhub server status` — report whether the server is running and healthy, including URL and log path when available.
- Server configuration management through the existing `evalhub config` workflow using a new `server_config_file` key:
- `evalhub config set server_config_file <path>` — validates the YAML file and stores a profile-scoped copy under `~/.config/evalhub/server/<profile>/config.yaml`.
- `evalhub config get server_config_file` / `show` — displays the stored value.
- `evalhub config unset server_config_file` — removes the stored config file and cleans up the profile directory.
- A new `--unfold` flag on `evalhub config get` to print the contents of file-based configuration values (currently only applicable to `server_config_file`).
- Support for TLS-enabled servers (health checks over HTTPS, `https://` URLs reported in `start`/`status` output).
## Requested documentation updates
- [ ] Add a new "Server" section to the CLI reference documenting `evalhub server run|start|stop|status`, including example invocations (foreground vs background) and description of PID/log file locations.
- [ ] Document the `server_config_file` config key: how to set/get/unset it, and the YAML validation requirements (must exist, be valid YAML, and be a mapping).
- [ ] Document the `--unfold` flag on `evalhub config get`, its mutual exclusivity with `--unmask`, and that it only applies to file-based config keys.
- [ ] Include examples covering: starting the server in the foreground, starting/stopping in the background, checking status, handling TLS, and removing the server config setting.
## References
- PR: https://github.com/eval-hub/eval-hub-sdk/pull/149
- Requested by: `@ruivieira`
EOF
)
gh issue create \
--repo "$repo" \
--title "Document evalhub server command and server_config_file config key from eval-hub-sdk PR `#149`" \
--body "$body" \
--assignee "ruivieira"Length of output: 2989 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="eval-hub/eval-hub.github.io"
# Check if the issue was already created despite the assignment error
gh issue list --repo "$repo" --search "Document evalhub server command" --json number,title,url,assigneesLength of output: 579 The issue was created successfully, but I couldn't assign it to you due to a permissions limitation with the integration. You'll need to self-assign it manually if desired.
It covers the new |
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
3 participants
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.
What and why
Add
evalhub servercommand group with run/start/stop/status and config management (set/show/unset), mirroring the mcp subcommand pattern.Set the server config file
config list does not unfold the yaml
To check the server yaml use config get with --unfold:
To start the server in foreground
To start the server in background:
This support tls:
To unset:
Closes # https://redhat.atlassian.net/browse/RHOAIENG-68000
Type
Testing
Breaking changes
No
Summary by CodeRabbit
Summary
New Features
evalhub servercommand group (run,start,stop,status).--unfold.Bug Fixes
Tests
servercommand lifecycle and config workflows.