Repository navigation
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the OpenHands agent harness within the tunix framework to ensure full compliance with MLPerf Training v6.1 rules. The changes include standardizing tool schemas, refining prompt templates for Qwen3.5, and implementing local tool execution logic to match the reference harness contract. These updates improve agent reliability and compatibility while maintaining the existing sandbox environment. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for OpenHands scaffolds, including local editor execution, structured task tracking, persistent bash sessions, and Qwen-style XML tool-calling formats. While the implementation is thorough and well-tested, several improvements are needed to align with the repository's style guide. Specifically, broad Exception blocks should be replaced with specific exceptions like OSError and json.JSONDecodeError when handling file operations and JSON parsing. Additionally, speculative hasattr and getattr calls on structured objects like workspace and CommandResult should be replaced with direct attribute access to adhere to the 'One well-lit path' philosophy.
| except Exception: # pylint: disable=broad-exception-caught | ||
| pass |
There was a problem hiding this comment.
The repository style guide explicitly advises against catching overly broad exceptions like bare Exception to swallow errors. Since this block is loading history from a JSON file, we should catch specific exceptions such as OSError and json.JSONDecodeError instead.
| except Exception: # pylint: disable=broad-exception-caught | |
| pass | |
| except (OSError, json.JSONDecodeError): | |
| pass |
References
- Use specific exceptions: Avoid catching or raising overly broad exceptions like bare Exception to swallow errors. (link)
| except Exception: # pylint: disable=broad-exception-caught | ||
| pass |
There was a problem hiding this comment.
Catching a broad Exception to swallow errors when saving history is discouraged by the style guide. Please catch OSError specifically, as it is the appropriate exception class for file writing and directory creation errors.
| except Exception: # pylint: disable=broad-exception-caught | |
| pass | |
| except OSError: | |
| pass |
References
- Use specific exceptions: Avoid catching or raising overly broad exceptions like bare Exception to swallow errors. (link)
| if isinstance(raw_range, str): | ||
| try: | ||
| view_range = json.loads(raw_range) | ||
| except Exception: # pylint: disable=broad-exception-caught |
There was a problem hiding this comment.
Catching a broad Exception when parsing view_range with json.loads is discouraged. Please catch json.JSONDecodeError specifically.
| except Exception: # pylint: disable=broad-exception-caught | |
| except json.JSONDecodeError: |
References
- Use specific exceptions: Avoid catching or raising overly broad exceptions like bare Exception to swallow errors. (link)
| if isinstance(parsed_commit, str): | ||
| try: | ||
| parsed_commit = json.loads(parsed_commit) | ||
| except Exception: |
There was a problem hiding this comment.
Catching a broad Exception when parsing parsed_commit with json.loads is discouraged. Please catch json.JSONDecodeError specifically.
| except Exception: | |
| except json.JSONDecodeError: |
References
- Use specific exceptions: Avoid catching or raising overly broad exceptions like bare Exception to swallow errors. (link)
| if isinstance(raw_list, str): | ||
| try: | ||
| task_list = json.loads(raw_list) | ||
| except Exception as e: |
There was a problem hiding this comment.
Catching a broad Exception when parsing task_list with json.loads is discouraged. Please catch json.JSONDecodeError specifically.
| except Exception as e: | |
| except json.JSONDecodeError as e: |
References
- Use specific exceptions: Avoid catching or raising overly broad exceptions like bare Exception to swallow errors. (link)
| if entry is None and hasattr(workspace, "entry"): | ||
| entry = getattr(workspace, "entry", None) |
There was a problem hiding this comment.
According to the repository style guide's 'One well-lit path' philosophy, speculative attribute accesses using hasattr and getattr with defaults should be avoided on structured contracts. If workspace is expected to have an entry attribute, we should access it directly or define a strict Protocol/interface rather than using defensive fallbacks.
| if entry is None and hasattr(workspace, "entry"): | |
| entry = getattr(workspace, "entry", None) | |
| if entry is None: | |
| entry = workspace.entry |
References
- One well-lit path (No defensive fallbacks): Never use speculative getattr(obj, 'field', default), dict.get('key', default) on structured schemas... Access attributes and keys directly (obj.field) on a single, strictly typed contract. (link)
| if getattr(res, "exit_code", 0) != 0: | ||
| logging.warning( | ||
| "[SWEEnv] Workspace setup exit code %s: %s", | ||
| res.exit_code, | ||
| res.stderr or res.stdout, | ||
| getattr(res, "stderr", None) or getattr(res, "stdout", None), | ||
| ) |
There was a problem hiding this comment.
Using speculative getattr with defaults on res (which is a CommandResult object returned by workspace.execute_command) violates the 'One well-lit path' philosophy in the style guide. Since res has a stable, strictly typed contract with exit_code, stderr, and stdout attributes, please access them directly.
| if getattr(res, "exit_code", 0) != 0: | |
| logging.warning( | |
| "[SWEEnv] Workspace setup exit code %s: %s", | |
| res.exit_code, | |
| res.stderr or res.stdout, | |
| getattr(res, "stderr", None) or getattr(res, "stdout", None), | |
| ) | |
| if res.exit_code != 0: | |
| logging.warning( | |
| "[SWEEnv] Workspace setup exit code %s: %s", | |
| res.exit_code, | |
| res.stderr or res.stdout, | |
| ) |
References
- One well-lit path (No defensive fallbacks): Never use speculative getattr(obj, 'field', default), dict.get('key', default) on structured schemas... Access attributes and keys directly (obj.field) on a single, strictly typed contract. (link)
| else: | ||
| logging.info("[SWEEnv] Successfully configured OpenHands workspace") | ||
| if isinstance(entry, dict) and not entry.get("base_commit"): | ||
| stdout = getattr(res, "stdout", None) |
There was a problem hiding this comment.
Using speculative getattr with defaults on res violates the 'One well-lit path' philosophy. Please access res.stdout directly.
| stdout = getattr(res, "stdout", None) | |
| stdout = res.stdout |
References
- One well-lit path (No defensive fallbacks): Never use speculative getattr(obj, 'field', default), dict.get('key', default) on structured schemas... Access attributes and keys directly (obj.field) on a single, strictly typed contract. (link)
| if getattr(result, "timeout_occurred", False) is True: | ||
| return True | ||
| exit_code = getattr(result, "exit_code", None) | ||
| return exit_code == -1 and elapsed >= 0.95 * float(timeout) |
There was a problem hiding this comment.
Using speculative getattr with defaults on result violates the 'One well-lit path' philosophy. Please access result.timeout_occurred and result.exit_code directly.
| if getattr(result, "timeout_occurred", False) is True: | |
| return True | |
| exit_code = getattr(result, "exit_code", None) | |
| return exit_code == -1 and elapsed >= 0.95 * float(timeout) | |
| if result.timeout_occurred is True: | |
| return True | |
| exit_code = result.exit_code | |
| return exit_code == -1 and elapsed >= 0.95 * float(timeout) |
References
- One well-lit path (No defensive fallbacks): Never use speculative getattr(obj, 'field', default), dict.get('key', default) on structured schemas... Access attributes and keys directly (obj.field) on a single, strictly typed contract. (link)
b9e3702 to
d9ec86c
Compare
2 participants
Summary
Minimal alignment of the OpenHands (
CodeActAgent) scaffold intunixwith the MLPerf Training v6.1 OpenHands agent harness contract (mlcommons/training_policies#599 and mlcommons/training_policies#598), keeping the existingopenhands-agent-server:1.44.1sandbox image and delegatingstr_replace_editorto the existingRepoEnvfile_editor:openhands-agent-server:1.44.1(no custom Docker image build):gcr.io/cloud-tpu-multipod-dev/sanbao/openhands-agent-server:1.44.1as the defaultOPENHANDS_SERVER_IMAGE, avoiding the custom:0.62PyInstaller/glibccontainer rebuild from feat(deepswe): align OpenHands prompts, tool schemas, parser, and execution semantics with MLPerf v6.1 #2620.examples/deepswe/template.py):execute_bash,think,finish,task_tracker,str_replace_editor) by settingenable_think: bool = Trueby default inget_openhands_tools()."security_risk"from"required"inexecute_bashandstr_replace_editor(while keeping"security_risk"in"properties"), matching the[[qwen35_openhands_agent_harness]]rule intraining_rules.adoc.examples/deepswe/template.py,examples/deepswe/swe_agent.py):OPENHANDS_CODEACT_SYSTEM_PROMPT(system_prompt.j2+ Qwen3.5<tools>header) andOPENHANDS_USER_PROMPT(user_prompt.j2/swe_default.j2).role="tool"(<tool_response>), emitsOPENHANDS_FAKE_USER_RESPONSE(role="user") when no tool call is made, and omits syntheticSteps Remaining/ token-exhaustion prompts inCodeActAgent.parser.py,swe_agent.py,eval_worker.py):\n<tool_call>\nand\n</tool_call>inBaseChatTemplateParser.str_replace_editorold_str/new_strparameters (parse_openhands_xml_action/parse_openhands_action_str)."stop": ["</function>"]for OpenHands scaffolds ineval_worker.py.openhands_utils.py,mlperf_base.sh,run_deepswe_dist.py,eval_deepswe.py):think,finish, andtask_tracker(view/plan) handlers instep_openhands, and strips"security_risk"before delegatingstr_replace_editorto the existingenv.env.step(action_obj).max_turns=30,step_timeout=60,reward_timeout=60,episode_timeout=1800).Test Plan
PYTHONPATH=. pytest tests/examples/template_test.py examples/deepswe/swe_agent_test.py tests/rl/agentic/parser/chat_template_parser/chat_template_parser_test.py(65 passed)