Skip to content

fix(python): address review nits for loop runner, step timeout, and trailing newline - #89

Merged
Tomer Glottmann (tomergee) merged 1 commit into
agent-substrate:mainfrom
sunnylovestiramisu:nit-fix
Oct 14, 2026
Merged

Tomer Glottmann (tomergee) merged 1 commit into
agent-substrate:mainfrom
sunnylovestiramisu:nit-fix

Conversation

@sunnylovestiramisu

@sunnylovestiramisu Sunny (sunnylovestiramisu) commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Address reviewer nitpicks following PR #80:

  1. Guard event loop closure in LoopRunner.stop():

    • loop.close() previously ran after self._thread.join(timeout=2.0) regardless of whether the background thread had finished. Calling close() on a still-running event loop raises RuntimeError.
    • Added if not self._thread.is_alive() and not self._loop.is_closed(): before closing.
  2. Wire FleetConfig.step_timeout_s to SandboxHandle.exec:

    • step_timeout_s was previously validated in FleetConfig but never referenced.
    • Added default_timeout_s: float = 120.0 to SandboxHandle, populated from FleetConfig.step_timeout_s during SandboxFleet._acquire_one().
    • Updated SandboxHandle.exec and SandboxHandle.exec_async to default timeout_s: Optional[float] = None, falling back to self.default_timeout_s when omitted.
  3. Code style / formatting:

    • Removed an extra blank line at the end of clients/python/src/ate_env/backend/__init__.py.

Testing

  • Added unit test test_fleet_step_timeout_applies_to_handle in test_exec_semantics.py to verify that step_timeout_s set on FleetConfig propagates correctly to SandboxHandle.default_timeout_s.
  • Ran full test suite:
    pytest clients/python/tests/
    # 119 passed, 29 skipped

…railing newline

- Guard loop.close() with thread liveness check in LoopRunner.stop
- Wire FleetConfig.step_timeout_s to default_timeout_s on SandboxHandle.exec
- Remove extra trailing blank line in backend/__init__.py
- Add unit test verifying fleet step_timeout_s propagation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 80ecc61. Approve.

All three nits from the #80 second pass are done: loop.close() runs only when the thread has exited, step_timeout_s reaches SandboxHandle.default_timeout_s and is the default for exec and exec_async, the blank line is gone. 119 tests pass, ruff is clean, the branch merges clean with main at a82a87f and with #88.

Blocking: none. Before merge: none.

Nit

  • handle.py:201-209 in #88 builds SandboxHandle without default_timeout_s, thus after both PRs land the async fleet's handles keep the 120 s default and ignore step_timeout_s. Whichever PR merges second passes default_timeout_s=self.config.step_timeout_s there too, with the same one-line test.

@tomergee
Tomer Glottmann (tomergee) merged commit df24cef into agent-substrate:main Oct 14, 2026
9 checks passed
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.

2 participants