Repository navigation
feat: opencode agent server - #1594
Conversation
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Glorf
left a comment
Glorf
left a comment
There was a problem hiding this comment.
Added some comments which IMO are worth the change, overall the PR is well structured and in the very good shape overall
| _npm_install(npm, version) | ||
|
|
||
| if not shutil.which("opencode"): | ||
| npm_bin_dir = subprocess.run( |
There was a problem hiding this comment.
npm bin was removed in npm 9; the bundled Node 22.15.0 ships npm 10, so this returns empty, the PATH fix no-ops, and ensure_opencode ends in the misleading "still not on PATH" error. Use npm prefix -g (+ /bin) or npm root -g.
|
|
||
| def model_post_init(self, __context: Any) -> None: | ||
| self.sem = Semaphore(self.config.concurrency) | ||
| ensure_opencode(self.config.opencode_version) |
There was a problem hiding this comment.
_npm_install runs with check=True, so a transient npm/network failure raises and takes down agent construction entirely. Might be good to add some retry logic.
| apiKey: ${nvidia_api_key} | ||
| models: | ||
| nvidia/qwen/qwen3-next-80b-a3b-instruct: {} | ||
| opencode_version: null |
There was a problem hiding this comment.
parse_opencode_session reads OpenCode's private sqlite schema. With opencode_version unpinned (null = latest), an upstream release can change that schema; parsing then silently returns nothing → empty response → reward 0 with only a warning. Pin a tested version in:
responses_api_agents/opencode_agent/configs/opencode_agent.yamlL30 (opencode_version: null)resources_servers/math_with_judge/configs/math_with_judge_opencode_agent.yaml— agent block L15–42 (noopencode_versionset → also defaults to latest)
+This also might affect score reproducibility after the agent harness gets updated, so IMO it's beneficial to have this freeze in place, wdyt?
There was a problem hiding this comment.
agree, addressed by pinning, thanks
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: SHIP
Clean, well-structured agent that closely follows the established claude_code_agent pattern. Cookie propagation, semaphore placement, subprocess lifecycle, config conventions, and Responses API output construction all match the repo's existing agents. No correctness, async, or API-compatibility concerns.
Two notes below (both author's-call); nothing blocking.
| inner = data["opencode_agent"]["responses_api_agents"]["opencode_agent"] | ||
| assert inner["entrypoint"] == "app.py" | ||
| assert inner["concurrency"] == 8 | ||
| assert inner["command"] == "opencode" |
There was a problem hiding this comment.
NOTE — No test for _run_opencode() subprocess lifecycle.
claude_code_agent has TestRunClaudeCode (two tests) that mock asyncio.create_subprocess_exec to verify env-var wiring, command construction, timeout behavior, and workspace cleanup. This agent's parsing and utility functions are well-tested, but the subprocess path is uncovered — meaning a regression in env assembly, the --thinking flag, or the finally: shutil.rmtree cleanup would not be caught.
Not blocking since the logic is straightforward and mirrors the existing claude_code_agent, but worth adding parity when convenient.
| return | ||
| config = self._deep_merge({}, copy.deepcopy(self.config.opencode_config)) | ||
| (work_dir / "opencode.json").write_text(json.dumps(config, indent=2)) |
There was a problem hiding this comment.
NOTE — _deep_merge into empty dict is a no-op; copy.deepcopy alone suffices.
_deep_merge({}, copy.deepcopy(x)) ≡ copy.deepcopy(x) — every key in override hits the else branch since base is empty, so the merge just assigns each top-level key. The deep copy already protects self.config.opencode_config from mutation.
Not wrong, just unnecessary complexity. Author's call.
| return | |
| config = self._deep_merge({}, copy.deepcopy(self.config.opencode_config)) | |
| (work_dir / "opencode.json").write_text(json.dumps(config, indent=2)) | |
| config = copy.deepcopy(self.config.opencode_config) |
Signed-off-by: Christian Munley <cmunley@nvidia.com>
github-project-automation
Bot
moved this from Review Todo
to Done
in NeMo Gym 0.4.0 - July 1
No description provided.