fix(runner): Preserve isolated verification artifacts - #2
Conversation
Run final checks with the same sanitized PATH as action steps and retain worktrees requested for evidence. Clean provisioned temp roots if setup fails, so failed allocation leaves no residual state. Co-Authored-By: Codex <noreply@openai.com>
Match the repository formatter so the isolated replay regression can pass the hosted CI format gate. Co-Authored-By: Codex <noreply@openai.com>
![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.
Reviewed commit: 2bdc1bc5b8
ℹ️ 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".
| if self._tempdir is not None: | ||
| self._tempdir.cleanup() | ||
| if self._tempdir.exists(): | ||
| shutil.rmtree(self._tempdir) |
There was a problem hiding this comment.
When an agent creates a directory without read/execute permissions (for example, mkdir locked && chmod 000 locked), the new plain shutil.rmtree raises PermissionError for non-root users. Because cleanup runs in the runner's finally block, this masks an otherwise valid RunResult and leaves the worktree behind; the previous TemporaryDirectory.cleanup() repaired permissions while deleting. Use similarly permission-tolerant removal for agent-controlled trees.
Useful? React with 👍 / 👎.
| except Exception: | ||
| self.cleanup() | ||
| raise |
There was a problem hiding this comment.
If copying is interrupted by KeyboardInterrupt or another BaseException, this handler is skipped and the raw mkdtemp directory has no finalizer, so a potentially large partial worktree remains permanently in the temp root. The former TemporaryDirectory object supplied that fallback cleanup; ensure the new allocation is also removed while propagating non-Exception interruptions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR tightens worktree isolation and artifact handling in the runner by ensuring verification runs under the same sanitized environment as action steps, and by retaining/cleaning worktree directories more predictably across success and failure paths.
Changes:
- Run
final_tests_pass()with a fully sanitizedPATH(matching executor behavior). - Change worktree provisioning to use an explicit
mkdtemp()root sokeep_worktreeruns don’t get GC-cleaned, while still cleaning up on provisioning failures. - Add integration/unit tests covering host-PATH independence, isolated worktree replays, and cleanup on copy failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_worktree.py | Adds a regression test ensuring failed provisioning cleans up the allocated temp root. |
| tests/test_runner_integration.py | Adds integration coverage for host-PATH independence and repeated runs creating distinct kept worktrees without mutating the source repo. |
| safe_mini/worktree.py | Switches from TemporaryDirectory to mkdtemp() plus explicit cleanup to preserve kept worktrees and cleanup on failures. |
| safe_mini/observation/policies.py | Ensures final verification runs with SANITIZED_PATH instead of inheriting host PATH. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if self._tempdir is not None: | ||
| self._tempdir.cleanup() | ||
| if self._tempdir.exists(): | ||
| shutil.rmtree(self._tempdir) | ||
| self._tempdir = None |
Summary
Verification