Skip to content

Fix secondary runs cleanup issue on windows - #758

Merged
Sun Haoran (haoranpb) merged 3 commits into
mainfrom
fix/secondary-run-fails-during-dir-cleanup
Jul 29, 2026
Merged

Fix secondary runs cleanup issue on windows#758
Sun Haoran (haoranpb) merged 3 commits into
mainfrom
fix/secondary-run-fails-during-dir-cleanup

Conversation

@haoranpb

@haoranpb Sun Haoran (haoranpb) commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

We clone plugin repos during runtime, the secondary run will have to delete the previously cloned git repo.

On windows this fails due to permission issues: https://stackoverflow.com/questions/1213706/what-user-do-python-scripts-run-as-in-windows.

We had similar problems before in nl2al, move the cleanup logic into operations utilities.


In order to catch similar issues in tests, lint and test job now run on both ubuntu and windows

Copilot AI review requested due to automatic review settings July 29, 2026 05:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Centralizes read-only directory cleanup to support repeated Windows runs.

Changes:

  • Adds a reusable remove_tree operation.
  • Uses it for repository cloning and NL2AL cleanup.
  • Extends clone cleanup testing with a read-only file.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_git_operations.py Adds read-only clone cleanup coverage.
src/bcbench/operations/git_operations.py Uses centralized cleanup before cloning.
src/bcbench/operations/filesystem_operations.py Implements read-only tree removal.
src/bcbench/operations/__init__.py Exports the new operation.
src/bcbench/evaluate/nl2al.py Reuses centralized cleanup logic.

Comment thread src/bcbench/operations/filesystem_operations.py Outdated
Comment thread tests/test_git_operations.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 29, 2026 06:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/test_git_operations.py:240

  • This does not exercise the Windows-specific recovery path in CI. On Ubuntu, a read-only file can still be unlinked from a writable parent, so this test also passes with the previous plain shutil.rmtree; the repository's pytest job runs only on ubuntu-latest (.github/workflows/CI.yml:18). Add a Windows test job or a platform-independent test that forces rmtree to invoke the onexc callback, otherwise the regression fix remains unverified.
    stale_file.chmod(0o444)

Copilot AI review requested due to automatic review settings July 29, 2026 06:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

@haoranpb
Sun Haoran (haoranpb) enabled auto-merge (squash) July 29, 2026 06:14
@haoranpb
Sun Haoran (haoranpb) merged commit ba3502d into main Jul 29, 2026
15 checks passed
@haoranpb
Sun Haoran (haoranpb) deleted the fix/secondary-run-fails-during-dir-cleanup branch July 29, 2026 06:22
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.

3 participants