Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d7514ceec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
configs/litellm/config.yaml
Outdated
| general_settings: | ||
| master_key: "os.environ/LITELLM_MASTER_KEY" | ||
| database_url: "os.environ/DATABASE_URL" |
There was a problem hiding this comment.
litellm_settings
When someone starts the compose stack, LiteLLM will ignore these two keys because its config schema expects master_key and database_url under litellm_settings (the upstream quick-start example puts both there). In practice that means the proxy comes up without DB-backed state and without enforcing the master key, so session-scoped virtual keys and request persistence cannot work.
Useful? React with 👍 / 👎.
configs/litellm/config.yaml
Outdated
| # LiteLLM Proxy Configuration for StackPerf Benchmarking | ||
| # Reference: https://docs.litellm.ai/docs/proxy/configs | ||
|
|
||
| model_list: [] |
There was a problem hiding this comment.
If an operator boots this stack and tries the included fireworks-kimi-k2-5-claude variant, LiteLLM has nothing to route to because model_list is empty and this commit adds no code that materializes configs/providers/*.yaml into proxy routes. The harness will send the alias kimi-k2-5, so benchmark traffic will be rejected instead of reaching Fireworks.
Useful? React with 👍 / 👎.
| { | ||
| "dashboard": { |
There was a problem hiding this comment.
Grafana file provisioning reads each JSON file here as a dashboard JSON model, but this file is wrapped in a top-level dashboard object, which is the Dashboard HTTP API payload shape instead. In the local stack that means the advertised LiteLLM Overview dashboard will not be imported on startup, leaving Grafana empty even though provisioning is configured.
Useful? React with 👍 / 👎.
configs/prometheus/prometheus.yml
Outdated
| - job_name: 'postgres' | ||
| static_configs: | ||
| - targets: ['postgres:5432'] |
There was a problem hiding this comment.
This scrape job targets postgres:5432, but that port speaks the PostgreSQL wire protocol, not Prometheus HTTP. As soon as Prometheus starts, the postgres target will remain DOWN on every scrape, so the local stack never collects any database metrics unless an exporter is added and scraped instead.
Useful? React with 👍 / 👎.
- LiteLLM: move master_key/database_url to litellm_settings, add model routes - Grafana: fix dashboard JSON format for file provisioning - Prometheus: remove invalid postgres wire protocol scrape target - Python: fix lint warnings (unused imports, blank line whitespace)
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadcb8e41f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| environment: | ||
| - LITELLM_MASTER_KEY=sk-stackperf-master-key-dev | ||
| - DATABASE_URL=postgresql://stackperf:stackperf-dev-password@postgres:5432/litellm | ||
| - LITELLM_SALT_KEY=stackperf-salt-key-dev-only | ||
| - STORE_MODEL_IN_DB=True |
There was a problem hiding this comment.
The sample proxy routes in configs/litellm/config.yaml:9-15 dereference FIREWORKS_BASE_URL and FIREWORKS_API_KEY, but this environment: block never injects either variable into the litellm container and does not use ${...} expansion, so copying .env.example still will not expose them. In the default local stack the proxy can start, but any request to kimi-k2-5 or glm-5 will fail at runtime because the upstream base URL/API key are missing inside the container.
Useful? React with 👍 / 👎.
| @main.command() | ||
| def config_validate() -> None: | ||
| """Validate all configuration files.""" | ||
| click.echo("Config validation: TODO") |
There was a problem hiding this comment.
This registers a top-level config-validate command that only prints Config validation: TODO and exits 0, while the contract published in docs/config-and-contracts.md:148-164 is bench config validate. As shipped, operators following the docs get No such command 'config', and even users who discover config-validate still get a false-success path that never loads or validates any YAML, so broken configs slip through unchecked.
Useful? React with 👍 / 👎.
- Remove .opensymphony/ metadata files from git tracking (Critical Issue #1) - Remove orphaned src/cli/diagnose.py - health commands exist in cli/commands/health.py (Critical Issue #3) - Fix hex_secret pattern to require 41+ chars to avoid matching 40-char git SHAs (Important Issue #4) Refs: COE-299
No description provided.