Skip to content

fix(interpreter): report file synchronization failures - #71

Draft
isaacbmiller wants to merge 1 commit into
isaac/rlm-fail-closedfrom
isaac/interpreter-ack-file-sync
Draft

fix(interpreter): report file synchronization failures#71
isaacbmiller wants to merge 1 commit into
isaac/rlm-fail-closedfrom
isaac/interpreter-ack-file-sync

Conversation

@isaacbmiller

Copy link
Copy Markdown

Stack: this PR is based on #68 (fix(rlm): fail closed on interpreter failures). Its own diff is one commit and three files.

1. Issue / repro

PythonInterpreter can report successful execution even when it fails to copy a sandbox file back to the host.

path.write_text("original_content")

with PythonInterpreter(enable_write_paths=[path]) as interpreter:
    result = interpreter.execute(
        "import os\nos.remove('/sandbox/sync_failure.txt')"
    )

On main, this returns an empty successful result while the host file still contains original_content. The sandbox mutation could not be synchronized, but the caller has no way to know.

2. Why this is the root cause

Execution results are acknowledged JSON-RPC requests, but file synchronization is a fire-and-forget notification:

sync_msg = _jsonrpc_notification("sync_file", ...)
stdin.write(sync_msg)

The runner then explicitly discards failures:

try {
  await Deno.writeFile(...);
} catch (error) { /* ignore sync errors */ }

Python returns the execution result before learning whether the requested persistent side effect succeeded.

3. How we know the fix addresses the root cause

The regression mounts a real temporary file, deletes its virtual sandbox copy, and then lets normal post-execution synchronization run.

Untouched main at 24ec85de4 prints:

''
original_content

This branch raises CodeInterpreterError containing Failed to sync file, and the assertion confirms that the original host file remains unchanged.

The full PythonInterpreter suite passes through the real Deno/Pyodide runner after the protocol change.

4. Why this is the concise fix

The existing request helper already writes a JSON-RPC request, waits for the matching response, validates its ID, and raises on an error response. File sync now uses that boundary:

self._send_request("sync_file", params, f"syncing {path}")

The runner returns one matching success or error response. There is no retry, polling loop, filesystem fallback, or second synchronization protocol.

5. Context needed to validate the change

For each configured write path, PythonInterpreter:

  1. mounts the host file into Pyodide;
  2. executes submitted Python;
  3. copies the virtual file back to the host when sync_files=True.

Step 3 is part of the observable result of execute(). A successful Python expression is not a successful interpreter operation when its requested host-side file changes were lost.

The parent PR matters to RLM specifically: once sync failure becomes a bare CodeInterpreterError, #68 ensures RLM propagates it rather than treating it as model-correctable Python.

6. What the fix does in the code

  • Sends each sync_file operation as an acknowledged JSON-RPC request.
  • Returns {synced: virtualPath} after a successful write.
  • Returns a structured JSON-RPC error when the virtual file cannot be read or the host file cannot be written.
  • Adds a real-runner regression for a missing virtual file.

Compatibility boundaries and downsides

  • Intentional behavior change: a failed sync now raises instead of returning false success.
  • One round trip per write path: execute() now waits for each file's acknowledgment. This adds small synchronization latency in exchange for knowing whether persistence succeeded.
  • Custom runners: a custom deno_command implementing DSPy's runner protocol must respond to sync_file requests. The bundled runner is updated in the same commit.
  • sync_files=False is unchanged: no synchronization request is sent.
  • Successful file writes are unchanged: content and mount paths are the same.
  • Multiple-file sync remains sequential, not atomic: if a later file fails, earlier files may already have synchronized. Transactional file updates are outside this PR.
  • No provider, tool, RLM prompt, or fallback change.

Validation

  • uv run --frozen pytest -q tests/primitives/test_python_interpreter.py --deno on the stacked branch — 55 passed
  • focused missing-virtual-file regression — passed
  • repository pre-commit hooks on all three changed files — passed
  • direct repro against untouched main — returns '' and leaves original_content; this branch raises

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