add openai tools and tests - #226
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@codex review |
| except ValueError: | ||
| exit_code = 0 | ||
| # strip the sentinel and exit code from output | ||
| output = output[:sentinel_idx] |
There was a problem hiding this comment.
Bug: Shell sentinel string in output causes truncation and wrong exit code
The sentinel detection uses a fixed string <<exit>> and searches for it anywhere in command output using if self._sentinel in output:. If a command's output contains this string (e.g., cat on a file containing it, or debug logging), the parser will detect the wrong sentinel. This causes stdout to be truncated at the first occurrence and the exit code to be incorrectly parsed from whatever text follows the false sentinel. The output is cut at output[:sentinel_idx], losing legitimate command output.
Additional Locations (1)
| return old, chunks, index, True | ||
|
|
||
| if index == orig_index: | ||
| raise DiffError(f"Nothing in this section - {index=} {self.lines[index]}") |
There was a problem hiding this comment.
Bug: IndexError raised instead of DiffError at EOF in parser
When _peek_next_section is called with self.index at or past the end of self.lines, the while loop at line 223 doesn't execute, and the check at line 291 if index == orig_index: evaluates to True. The error message then tries to access self.lines[index] for debugging context, but index is out of bounds, causing an IndexError instead of the intended DiffError. This can occur when parsing malformed patches where a standalone "@@" line appears without any following content before EOF.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| # Check for directory traversal | ||
| # Use base_path + os.sep to prevent sibling directory prefix bypass | ||
| # e.g., /tmp/myapp_sibling shouldn't match base_path /tmp/myapp | ||
| if full_path != self.base_path and not full_path.startswith(self.base_path + os.sep): |
There was a problem hiding this comment.
The traversal guard only checks full_path.startswith(self.base_path + os.sep) on the normalized path without resolving symlinks, so any symlink inside base_path can point outside the sandbox and still pass the prefix test. For example, if base_path/escape is a symlink to /etc, a diff targeting escape/passwd will be accepted because the joined path begins with base_path, but open/write will follow the symlink to /etc/passwd, allowing apply_patch to read or overwrite files outside the intended root. Use os.path.realpath (or commonpath) before the prefix check to close this escape.
Useful? React with 👍 / 👎.
| if self._sentinel in output: | ||
| # Extract exit code from sentinel line | ||
| sentinel_idx = output.index(self._sentinel) | ||
| # Find the exit code after the sentinel | ||
| after_sentinel = output[sentinel_idx + len(self._sentinel) :] |
There was a problem hiding this comment.
Command output is scanned with if self._sentinel in output and the first occurrence is treated as the terminator, so any command that prints <<exit>> in its own stdout (e.g., printf '<<exit>> hello') will be truncated at that substring and the following user data will be parsed as the exit code. The tool will report incomplete stdout and a bogus exit status even though the command succeeded. Consider only stripping the last occurrence of the sentinel you append (e.g., by splitting on the final line written by the wrapper) or using a delimiter unlikely to occur in user output.
Useful? React with 👍 / 👎.
| old_session = self._session | ||
| if old_session._timed_out: | ||
| restart_message = "Previous session timed out. Session auto-restarted." | ||
| elif old_session._process.returncode is not None: |
There was a problem hiding this comment.
Bug: Accessing uninitialized _process after failed session start
If start() fails (e.g., due to system resource exhaustion or permission errors), self._session is set to the _BashSession instance before calling start(), but _process is never initialized. On subsequent calls to _ensure_session(), the code enters the restart branch because is_alive() returns False (short-circuiting on _started), but then tries to access old_session._process.returncode at line 222, which raises AttributeError since _process was never assigned. The code needs to check whether _process exists before accessing it, or guard against the _started being False case.
Additional Locations (1)
This branch had an error being deployed
2 participants
Note
Introduce OpenAI-spec
apply_patchandshelltools with full implementations and extensive tests; minor assert cleanup and lint config update.hud/tools/apply_patch.py: Implement OpenAI apply_patch tool (create/update/delete files, V4A diff parsing, path validation, commit application).hud/tools/shell.py: Implement OpenAI shell tool with bash session management, timeouts, auto-restart, and structured outputs.hud/tools/tests/test_apply_patch.py: Unit tests covering parsing, path validation (including traversal protections), commit application, and tool API flows.hud/tools/tests/test_shell.py: Unit tests for session lifecycle, timeouts, auto-restart messaging, command execution, and result formatting.# noqa: S101from asserts inhud/clients/mcp_use.py,hud/rl/learner.py,hud/utils/strict_schema.py.pyproject.tomlto ignoreS101and adjust lint/test config.Written by Cursor Bugbot for commit b915f24. This will update automatically on new commits. Configure here.