Skip to content

fix(tools): correct Python-fallback bugs in calculator, file tools, a…#236

Open
TacoengineerIT wants to merge 1 commit intoopen-jarvis:mainfrom
TacoengineerIT:main
Open

fix(tools): correct Python-fallback bugs in calculator, file tools, a…#236
TacoengineerIT wants to merge 1 commit intoopen-jarvis:mainfrom
TacoengineerIT:main

Conversation

@TacoengineerIT
Copy link
Copy Markdown

Summary

Fixed 9 real bugs across 4 source files discovered via static analysis and
manual code review. All bugs affect the Python fallback path (active when the
Rust extension is not compiled); two are cross-platform regressions that fail
on Windows.

Problem

1. calculator.py — 6 bugs in the Python AST fallback

Expression Expected Actual (before fix)
safe_eval("2^10") 1024.0 ValueError — Python ^ is XOR, not power
safe_eval("ln(e)") 1.0 ValueError: Unknown function: ln
safe_eval("1/0") math.inf ZeroDivisionError unhandled
safe_eval("2 +") ValueError SyntaxError — broke pytest.raises(ValueError)
safe_eval("x + 1") raises "unknown variable" Capital U broke regex match
CalculatorTool.execute("1/0") success=True, content="inf" success=False

2. file_read.py / file_write.py — cross-platform path check

_is_path_allowed used str(path).startswith(str(d) + "/") with a hardcoded
/ separator. On Windows (os.sep = \) this never matched, so
allowed_dirs always denied access — even to permitted paths.

3. git_tool.py — unhandled NotADirectoryError on Windows

_run_git did not catch NotADirectoryError (WinError 267), raised by
subprocess.run when cwd is a non-existent directory. The exception
propagated instead of returning ToolResult(success=False).

Solution

  • calculator: pre-process ^**, add ln alias, convert SyntaxErrorValueError, return math.inf on
    division by zero, fix error message casing
  • file_read / file_write: replace startswith(str(d) + "/") with Path.is_relative_to(d) (OS-agnostic, Python ≥
    3.9)
  • git_tool: add except NotADirectoryError handler in _run_git

Testing

  • Existing tests pass (uv run pytest tests/ -v)
  • tests/tools/test_calculator.py25/25 (was 6 failed)
  • tests/tools/test_file_read.pytest_allowed_dirs_permits passes
  • tests/tools/test_file_write.pytest_allowed_dirs_permits passes
  • tests/tools/test_git_tool.pytest_invalid_repo_path passes

Notes

Found via static analysis + manual code review of the full codebase. Bugs live
in the Python fallback path — invisible when openjarvis_rust is compiled, but
they surface immediately for contributors who skip maturin develop.

…nd git tool

- calculator: add `ln` alias for `math.log`, replace `^` with `**` before
  AST parsing so caret-as-power works, convert SyntaxError → ValueError for
  consistent error handling, and return `math.inf` on division by zero
  (matching the documented meval behaviour) instead of raising an exception
- file_read / file_write: replace the hardcoded `str(path).startswith(str(d) + "/")
  guard with `Path.is_relative_to()` so allowed-directory checks work on
  Windows (and any OS whose separator is not `/`)
- git_tool: catch `NotADirectoryError` raised by `subprocess.run` on Windows
  when `cwd` points to a non-existent path, returning a proper error result

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant