fix(resume): add Pi models.json regeneration on URL change#19
fix(resume): add Pi models.json regeneration on URL change#19rounakbende10 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe resume endpoint optionally replaces old server URLs across job JSON files and regenerates pi ChangesResume URL configuration updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fda7c77 to
6102e45
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
src/coding_agent_bench/api.py (3)
60-60: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument JSON-only replacement behavior.
The script updates only files ending in
.json(Line 528), so “across all job files” overstates the request contract. Clarify this as “all job JSON files” or update the traversal.🤖 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 `@src/coding_agent_bench/api.py` at line 60, Update the `server_url` field description to state that the replacement applies to all job JSON files, matching the script’s `.json`-only traversal behavior; do not change traversal unless intentionally expanding the request contract.
525-542: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not exit before regenerating Pi models.
When no matching old host is found, the
exit(0)at Line 524 prevents Lines 537-542 from running. Thus a resume with an already-current URL—or a host outside the derived domain—never updatesmodels.json, despite the newserver_urlbeing supplied. Make URL replacement conditional without terminating the script, then always perform Pi model regeneration when applicable.🤖 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 `@src/coding_agent_bench/api.py` around lines 525 - 542, Update the URL replacement flow before the Pi regeneration block so the no-matching-host case does not call exit(0). Make replacement conditional, but allow execution to continue into the mounts loop and regenerate models.json whenever the selected agent is Pi and its mount target matches models.json.
506-513: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed or scheme-less server URLs here.
urlparse(...).netlocis empty for inputs likenot-a-urlorexample.com, sonew_domainbecomes""; the host filter then matches every host and can rewrite job JSONs tohttps:///v1. Validate the scheme and hostname before building the replacement script.🤖 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 `@src/coding_agent_bench/api.py` around lines 506 - 513, Validate req.server_url before deriving new_host and new_domain: require a supported URL scheme and a non-empty parsed hostname, rejecting malformed or scheme-less values instead of generating replacement lines. Use the validated hostname when constructing new_domain, while preserving the existing replacement-script flow for valid URLs.
🧹 Nitpick comments (1)
src/coding_agent_bench/api.py (1)
542-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unnecessary f-string prefix.
Ruff reports F541 because this string contains no interpolated expressions; removing the
fprefix avoids the lint failure.🤖 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 `@src/coding_agent_bench/api.py` at line 542, Remove the unnecessary f-string prefix from the string containing the Pi models.json regeneration command in the surrounding code, since it has no interpolation. Preserve the command contents and behavior unchanged.Source: Linters/SAST tools
🤖 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/coding_agent_bench/api.py`:
- Around line 537-542: Update the generated Pi mount-file logic around
PiAgentConfig.configure to reuse the persisted complete server_url, including
its scheme and path, instead of constructing a new HTTPS URL from new_host.
Populate contextWindow from the configured model_max_len rather than hardcoding
262000, while preserving the existing model and provider fields.
---
Outside diff comments:
In `@src/coding_agent_bench/api.py`:
- Line 60: Update the `server_url` field description to state that the
replacement applies to all job JSON files, matching the script’s `.json`-only
traversal behavior; do not change traversal unless intentionally expanding the
request contract.
- Around line 525-542: Update the URL replacement flow before the Pi
regeneration block so the no-matching-host case does not call exit(0). Make
replacement conditional, but allow execution to continue into the mounts loop
and regenerate models.json whenever the selected agent is Pi and its mount
target matches models.json.
- Around line 506-513: Validate req.server_url before deriving new_host and
new_domain: require a supported URL scheme and a non-empty parsed hostname,
rejecting malformed or scheme-less values instead of generating replacement
lines. Use the validated hostname when constructing new_domain, while preserving
the existing replacement-script flow for valid URLs.
---
Nitpick comments:
In `@src/coding_agent_bench/api.py`:
- Line 542: Remove the unnecessary f-string prefix from the string containing
the Pi models.json regeneration command in the surrounding code, since it has no
interpolation. Preserve the command contents and behavior unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f761a8f2-c990-440e-a3e8-7e47e7ca33b2
📒 Files selected for processing (1)
src/coding_agent_bench/api.py
| "# Regenerate Pi mount file with new URL", | ||
| "agent = c.get('agents', [{}])[0]", | ||
| "mounts = c.get('environment', {}).get('mounts', [])", | ||
| "for m in mounts:", | ||
| " src, tgt = m.get('source',''), m.get('target','')", | ||
| f" if agent.get('name') == 'pi' and 'models.json' in tgt: json.dump({{'providers': {{'vllm': {{'baseUrl': 'https://' + new_host + '/v1', 'api': 'openai-completions', 'apiKey': 'NONE', 'models': [{{'id': agent.get('model_name',''), 'name': agent.get('model_name',''), 'contextWindow': 262000}}]}}}}}}, open(src, 'w')); print('Regenerated Pi models.json')", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the canonical Pi configuration values.
PiAgentConfig.configure uses the complete server_url (including scheme/path) and the configured model_max_len. This regeneration instead forces https://, discards any URL path, and hardcodes contextWindow to 262000, which can break deployments using HTTP, a URL prefix, or a custom context window. Reuse the persisted URL/configuration values.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 542-542: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@src/coding_agent_bench/api.py` around lines 537 - 542, Update the generated
Pi mount-file logic around PiAgentConfig.configure to reuse the persisted
complete server_url, including its scheme and path, instead of constructing a
new HTTPS URL from new_host. Populate contextWindow from the configured
model_max_len rather than hardcoding 262000, while preserving the existing model
and provider fields.
Pi agent stores the server URL in a bind-mounted models.json file that doesn't persist to MinIO. When resuming with a new server_url, the URL replacement walks all JSON files but misses this mount file. Added Pi models.json regeneration inline in the URL replacement script.
6102e45 to
277a4c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/coding_agent_bench/api.py`:
- Around line 542-543: Remove the unnecessary f-string prefixes from the two
command strings in the agent configuration generation logic, including the lines
handling Pi models.json and Codex config.toml. Keep their contents and behavior
unchanged.
- Around line 537-543: Update the server URL handling before the Pi/Codex
regeneration block so URL replacement is conditional rather than exiting when no
URLs match. When req.server_url is set, always continue into the mounts loop and
regenerate Pi models.json or Codex config.toml using the new host, regardless of
whether any prior URLs were replaced; preserve replacement behavior when matches
exist.
- Line 543: Update the Codex config generation in the agent config handling to
parse the existing config.toml, preserve unrelated settings, and modify only
api.base_url and model.model_id. Serialize the updated TOML using the project’s
proper TOML writer or escaping mechanism so new_host and agent.get('model_name',
'') remain valid even with quotes or newlines, rather than overwriting the file
with a handcrafted string.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 92a80a33-9bff-4036-943c-c4202edca7ab
📒 Files selected for processing (1)
src/coding_agent_bench/api.py
| f" if agent.get('name') == 'pi' and 'models.json' in tgt: json.dump({{'providers': {{'vllm': {{'baseUrl': 'https://' + new_host + '/v1', 'api': 'openai-completions', 'apiKey': 'NONE', 'models': [{{'id': agent.get('model_name',''), 'name': agent.get('model_name',''), 'contextWindow': 262000}}]}}}}}}, open(src, 'w')); print('Regenerated Pi models.json')", | ||
| f" if agent.get('name') == 'codex' and 'config.toml' in tgt: open(src, 'w').write('[api]\\nbase_url = \"https://' + new_host + '\"\\napi_key = \"sk-no-key\"\\n[model]\\nmodel_id = \"' + agent.get('model_name','') + '\"\\n'); print('Regenerated Codex config.toml')", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unnecessary f-string prefixes.
Ruff F541 reports that these strings contain no interpolation; removing the f prefixes will clear the lint errors.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 542-542: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 543-543: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@src/coding_agent_bench/api.py` around lines 542 - 543, Remove the unnecessary
f-string prefixes from the two command strings in the agent configuration
generation logic, including the lines handling Pi models.json and Codex
config.toml. Keep their contents and behavior unchanged.
Source: Linters/SAST tools
| "for m in mounts:", | ||
| " src, tgt = m.get('source',''), m.get('target','')", | ||
| f" if agent.get('name') == 'pi' and 'models.json' in tgt: json.dump({{'providers': {{'vllm': {{'baseUrl': 'https://' + new_host + '/v1', 'api': 'openai-completions', 'apiKey': 'NONE', 'models': [{{'id': agent.get('model_name',''), 'name': agent.get('model_name',''), 'contextWindow': 262000}}]}}}}}}, open(src, 'w')); print('Regenerated Pi models.json')", | ||
| f" if agent.get('name') == 'codex' and 'config.toml' in tgt: open(src, 'w').write('[api]\\nbase_url = \"https://' + new_host + '\"\\napi_key = \"sk-no-key\"\\n[model]\\nmodel_id = \"' + agent.get('model_name','') + '\"\\n'); print('Regenerated Codex config.toml')", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve and safely serialize the existing Codex configuration.
open(src, 'w').write(...) discards unrelated Codex settings, while unescaped model_name and new_host can corrupt the generated TOML when they contain quotes or newlines. Parse and update only base_url/model_id, then serialize with proper TOML escaping.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 543-543: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@src/coding_agent_bench/api.py` at line 543, Update the Codex config
generation in the agent config handling to parse the existing config.toml,
preserve unrelated settings, and modify only api.base_url and model.model_id.
Serialize the updated TOML using the project’s proper TOML writer or escaping
mechanism so new_host and agent.get('model_name', '') remain valid even with
quotes or newlines, rather than overwriting the file with a handcrafted string.
The URL replacement script had 'if not hosts: exit(0)' which killed the entire script before reaching Pi models.json / Codex config.toml mount file regeneration when the harness stores URLs in mount files instead of env vars.
1 participant
Problem
Pi agent stores the server URL in a bind-mounted
models.jsonfile, not in env vars. The URL replacement in the resume endpoint only walks JSON files in the job dir — it doesn't regenerate Pi's mount file.Fix
After replacing URLs across all JSON files, check if the agent is Pi and regenerate
/app/models.jsonwith the new URL. Added inline in the existing URL replacement script.Also includes the HOME=/tmp fix from PR #18 for mc permissions.