fix(benchmarks): parameterize hardcoded ports across all 4 benchmarks#114
Merged
Conversation
- REGISTRY_PORT was a literal `REGISTRY_PORT=8001` in eval.sh, ignoring
any pre-set env var; now resolved as
${REGISTRY_PORT:-${DYNACONF_SERVER_PORTS__REGISTRY:-8001}}, matching
the pattern already used by benchmarks/m3.
- eval.sh's AppWorld health check was hardcoded to poll port 8000
regardless of DYNACONF_SERVER_PORTS__ENVIRONMENT_URL, so a custom
environment_url port would start fine but time out the check. Added
APPWORLD_ENV_PORT, resolved the same way and exported before `cuga
start appworld` runs.
- compare.sh now sources load_env.sh and resolves both ports the same
way so its kill_port_processes cleanup targets the right ports.
Part of #17.
REGISTRY_PORT was a literal `REGISTRY_PORT=8001` in eval.sh, ignoring
any pre-set env var. Now resolved as
${REGISTRY_PORT:-${DYNACONF_SERVER_PORTS__REGISTRY:-8001}} after
load_env.sh, matching the pattern already used by benchmarks/m3.
compare.sh now sources load_env.sh and resolves REGISTRY_PORT the same
way so its kill_port_processes cleanup targets the right port.
FASTAPI_PORT (bpo's own mock app, 8095) is left hardcoded — it's also
baked into benchmarks/bpo/mcp_servers/bpo.yaml as the registry's
OpenAPI URL, so making it configurable needs YAML templating too;
tracked separately in #113.
Part of #17.
REGISTRY_PORT was a literal `REGISTRY_PORT=8001` in eval.sh, ignoring
any pre-set env var. Now resolved as
${REGISTRY_PORT:-${DYNACONF_SERVER_PORTS__REGISTRY:-8001}} after
load_env.sh, matching the pattern already used by benchmarks/m3.
compare.sh now sources load_env.sh and resolves REGISTRY_PORT the same
way so its kill_port_processes cleanup targets the right port.
FASTAPI_PORT (oak's own mock app, 8090) is left hardcoded — it's also
baked into benchmarks/oak_health_insurance/oak_mcp_servers.yaml as the
registry's OpenAPI URL, so making it configurable needs YAML
templating too; tracked separately in #113.
Part of #17.
load_eval_config() called load_dotenv(..., override=True) for both config/global.env and the benchmark's own .env file, so any env var the caller pre-set (e.g. DYNACONF_SERVER_PORTS__APIS_URL before calling load_eval_config for appworld) got silently overwritten by whatever the .env file hardcoded (appworld.env hardcodes APIS_URL=9111). This was backwards from load_env.sh's bash-side convention, which deliberately loads without override so pre-set env vars win. Switched to parsing both files with dotenv_values() and merging them in-memory (benchmark file still overrides global.env), then only os.environ.setdefault()-ing keys that aren't already set — so a caller's pre-set env var always wins over both files, while the global->benchmark override layering other settings rely on (DYNACONF_ADVANCED_FEATURES__REGISTRY differs between global.env and every benchmark's .env) is preserved. Closes #17.
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughBenchmark eval.sh and compare.sh scripts for appworld, bpo, and oak_health_insurance now derive REGISTRY_PORT (and AppWorld's environment server port) from environment variables after loading configuration, instead of hardcoding them, and export corresponding DYNACONF_SERVER_PORTS__* values used by cleanup/readiness logic. Additionally, config_loader.py's load_eval_config no longer overrides pre-set environment variables when loading .env files, with new regression tests covering precedence behavior. ChangesConfigurable Server Ports
Estimated code review effort: 2 (Simple) | ~15 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
benchmarks/helpers/config_loader.py (1)
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the
None-value edge case.
dotenv_valuesreturnsNonefor keys present in the file but without a value (e.g., a bareKEYline). Theif value is not Noneguard correctly preventsos.environ.setdefault(key, None)from raising aTypeError. However, no test currently pins this behavior — a future refactor could accidentally drop the guard.🧪 Suggested test
def test_none_value_from_dotenv_is_skipped(monkeypatch): def _fake_dotenv_with_none(path): if path.name == "global.env": return {} return {"EMPTY_KEY": None} monkeypatch.setattr(config_loader, "dotenv_values", _fake_dotenv_with_none) monkeypatch.delenv("EMPTY_KEY", raising=False) config_loader.load_eval_config("fake_benchmark") assert "EMPTY_KEY" not in os.environ🤖 Prompt for 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. In `@benchmarks/helpers/config_loader.py` around lines 46 - 47, Add a regression test for the `None`-value path in `load_eval_config` in `config_loader.py`: mock `dotenv_values` so one merged entry returns `{"EMPTY_KEY": None}`, clear `EMPTY_KEY` from `os.environ`, call `load_eval_config`, and assert the key is not added. This pins the `if value is not None` guard in the `merged_values` loop and protects against future refactors that might pass `None` into `os.environ.setdefault`.
🤖 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.
Nitpick comments:
In `@benchmarks/helpers/config_loader.py`:
- Around line 46-47: Add a regression test for the `None`-value path in
`load_eval_config` in `config_loader.py`: mock `dotenv_values` so one merged
entry returns `{"EMPTY_KEY": None}`, clear `EMPTY_KEY` from `os.environ`, call
`load_eval_config`, and assert the key is not added. This pins the `if value is
not None` guard in the `merged_values` loop and protects against future
refactors that might pass `None` into `os.environ.setdefault`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 662c7e5f-5c92-4ec6-bac7-e7fe55611e32
📒 Files selected for processing (8)
benchmarks/appworld/compare.shbenchmarks/appworld/eval.shbenchmarks/bpo/compare.shbenchmarks/bpo/eval.shbenchmarks/helpers/config_loader.pybenchmarks/helpers/tests/test_config_loader.pybenchmarks/oak_health_insurance/compare.shbenchmarks/oak_health_insurance/eval.sh
…oader Addresses CodeRabbit's docstring-coverage pre-merge check and nitpick on PR #114: pins the `if value is not None` guard in config_loader.load_eval_config so a bare `KEY` line in a .env file (dotenv_values -> None) can't reach os.environ.setdefault().
The earlier fix for issue #17 (override=True clobbering pre-set env vars) landed on benchmarks/helpers/config_loader.py, but that module had zero real importers other than a dead eager re-export in benchmarks/helpers/__init__.py. Every benchmark entrypoint actually imports load_eval_config from a separate root-level config_loader package (sys.path.insert(0, project_root); from config_loader import load_eval_config), which still had the original bug — confirmed live when a concurrent appworld test with DYNACONF_SERVER_PORTS__APIS_URL overridden still resolved to the hardcoded appworld.env value. - Deleted the dead benchmarks/helpers/config_loader.py + its test, and the unused load_eval_config re-export from benchmarks/helpers/__init__.py. - Applied the same dotenv_values()-merge-then-setdefault fix to the real config_loader/loader.py, moving the regression test to tests/test_config_loader.py. Closes #17 (for real this time).
…8001
appworld_eval.py and eval_appworld_sdk.py both called
requests.get("http://localhost:8001/api/reset") literally, ignoring
REGISTRY_PORT/DYNACONF_SERVER_PORTS__REGISTRY entirely. Confirmed live:
running --agent cuga with REGISTRY_PORT=18001 crashed with
ConnectionError (port 8001 refused) even though the registry was up
and healthy on 18001.
Added the same _get_registry_base_url() helper already used correctly
by appworld_eval_react.py/appworld_eval_codeact.py (env var first,
settings.server_ports fallback, then the literal default) to both
files and used it for the reset call.
Part of #17.
AnkitaNaik
approved these changes
Jul 13, 2026
AnkitaNaik
left a comment
AnkitaNaik
left a comment
Collaborator
There was a problem hiding this comment.
No additional comments for the PR. The PR is good to merge.
Merged
9 tasks
haroldship
added a commit
that referenced
this pull request
Jul 16, 2026
… setting in global.env (#126) fix(config): stop global.env from silently overriding every benchmark's registry setting config/global.env has set DYNACONF_ADVANCED_FEATURES__REGISTRY=false since this repo's first commit, contradicting every benchmark's own .env (m3, appworld, bpo, oak_health_insurance all set it to true). This was masked for months by config_loader/loader.py's override=True behavior, which let the benchmark-specific file always win. #114 changed that loader to os.environ.setdefault (for an unrelated port-override use case), which now respects whatever the shell already exported - and load_env.sh's own no-override, global-loads-first order has always resolved that value to false. Net effect: every benchmark silently gets zero tools. - Remove the DYNACONF_ADVANCED_FEATURES__REGISTRY=false line and its stale "tools loaded directly from containers" comment (that direct-load mechanism doesn't exist anywhere in this repo's history) - No replacement default is set here on purpose: each benchmark's own .env already declares the value it wants Fixes #124
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
2 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.
Bug Fix Pull Request
Related Issue
Fixes #17
Related: #113 (follow-up, out of scope here)
Description
REGISTRY_PORTwas hardcoded as a literal assignment in appworld/bpo/oak'seval.sh/compare.sh(m3 already had this fixed), and the realconfig_loader/loader.py(imported by every benchmark entrypoint) clobbered any pre-set env var with whatever a benchmark's.envfile hardcoded. A live concurrent-run test also uncovered a hardcodedlocalhost:8001registry call in appworld's default (cuga) evaluator paths. Together these blocked running any of the 4 benchmarks — or two copies of the same benchmark side by side — on custom ports via env vars.Type of Changes
Root Cause
Three independent causes:
appworld/bpo/oak'seval.sh/compare.shhardcodedREGISTRY_PORT=8001as a literal shell assignment (appworld additionally hardcoded its AppWorld env-server health check to port8000), so settingREGISTRY_PORT/DYNACONF_SERVER_PORTS__REGISTRYbefore invoking these scripts had no effect.benchmarks/m3already had the fix for this.config_loader/loader.py'sload_eval_config()(the module every benchmark entrypoint actually imports viasys.path.insert(0, project_root); from config_loader import load_eval_config— notbenchmarks/helpers/config_loader.py, a dead duplicate with zero real importers, now deleted) calledload_dotenv(..., override=True)for bothconfig/global.envand the benchmark's own.env, so a caller's pre-set env var (e.g.DYNACONF_SERVER_PORTS__APIS_URLbefore callingload_eval_configfor appworld) got silently overwritten by whatever the.envfile hardcoded (appworld.envhardcodesAPIS_URL=9111). This was backwards from the bash-side convention inload_env.sh, which deliberately loads without override so pre-set env vars win.benchmarks/appworld/appworld_eval.pyandeval_appworld_sdk.pyboth calledrequests.get("http://localhost:8001/api/reset")literally, ignoringREGISTRY_PORTentirely — found only by live-testing appworld's defaultcugaagent path with a non-default registry port, which crashed withConnectionError.Solution
REGISTRY_PORT(and AppWorld's env-server port, via newAPPWORLD_ENV_PORT) inappworld/bpo/oak'seval.sh/compare.sh, mirroring the existing m3 pattern:"${VAR:-${DYNACONF_SERVER_PORTS__X:-default}}", exported before any server starts.config_loader/loader.py'sload_eval_config()to parse both.envfiles withdotenv_values()and merge them in-memory (benchmark file still overridesglobal.env, preserving existing layering likeDYNACONF_ADVANCED_FEATURES__REGISTRY), then onlyos.environ.setdefault()keys not already set — so a caller's pre-set env var always wins over both files. Deleted the deadbenchmarks/helpers/config_loader.pyduplicate (and its unused re-export frombenchmarks/helpers/__init__.py) that the original fix had mistakenly landed on._get_registry_base_url()helper (env var first,settings.server_portsfallback, then the literal default — matching the pattern already correct inappworld_eval_react.py/appworld_eval_codeact.py) toappworld_eval.pyandeval_appworld_sdk.py, replacing the hardcoded/api/resetcall.Not in scope:
settings.server_ports.demo(cuga-agent's Gradio UI, default port 7860) was checked and is not hardcoded anywhere in this repo — it already passes through Dynaconf env vars untouched, no fix needed.Deferred to #113: bpo/oak/appworld's app-specific ports are also hardcoded into MCP server YAML (
bpo/mcp_servers/bpo.yaml,oak_health_insurance/oak_mcp_servers.yamlforFASTAPI_PORT;appworld/mcp_servers_appworld.yamlforapis_url, 12 occurrences) as the registry's OpenAPI URLs — making these configurable needs YAML templating, tracked separately to keep this PR focused and low-risk.Testing
Added
tests/test_config_loader.pycovering: caller-preset env vars win over both.envfiles, benchmark.envstill overridesglobal.envwhen unset, global/benchmark-only keys are still applied, and a bareKEY-with-no-value.envline doesn't reachos.environ.setdefault(). Ranjust ci(444 passed, 2 skipped [vendor not installed, pre-existing], bandit clean, no known vulnerabilities) andbash -non every edited shell script.Manual concurrency tests — two AppWorld tasks run concurrently (one per instance), started 10s apart, one instance on 4 custom ports (
REGISTRY_PORT,DYNACONF_SERVER_PORTS__ENVIRONMENT_URL,DYNACONF_SERVER_PORTS__APIS_URL,DYNACONF_SERVER_PORTS__DEMO), the other on all defaults:--agent reactbaseline (before the config_loader/hardcoded-8001 fixes above were found): both instances started cleanly on fully independent ports (verified vialsof, no collisions), ran their full evaluation lifecycle, and exited 0. This test didn't exerciseappworld_eval.py's registry call at all (react/codeact already resolve the registry port correctly), so it didn't catch bug fix(m3): repair harness bugs that artificially zeroed CUGA M3 pass rate #3 above.--sdk --agent cuga(the default agent, via the SDK evaluator): this run is what originally surfaced bugs Solve conflict between .env and command line #2 and fix(m3): repair harness bugs that artificially zeroed CUGA M3 pass rate #3 — the custom-port instance crashed withConnectionError: ... port 8001 ... Connection refusedeven though its registry was healthy on port 18001, andapis_urlsilently stuck at the hardcoded9111instead of the requested override. After fixing both, re-ran the identical test: both instances again started on fully independent ports with no collisions or connection errors, ran to completion, and exited 0. (Task-level: both AppWorld tasks themselves didn't pass — one on keyword-match, one hit an external Groq rate-limit from running two concurrent LLM calls — both unrelated to the port fix; harness/infra-level success, which is what this test targets, was confirmed for all 4 ports.)A pre-existing, unrelated gap was also observed during testing:
appworld/m3/oak's registry cleanup only kills the tracked wrapper PID, not theuv run --no-sync registrysubprocess it spawns, occasionally leaving an orphaned registry process bound to the port after a run (bpo/eval.shalready has apkill -ffallback for this that the other three lack). Not fixed here — out of scope for port configurability — but worth a follow-up issue.Checklist
Summary by CodeRabbit