[mnriem]

Description

Use directory handles for FileHelper.delete() and recursive enumeration, preserving contained parent symlinks, trusted root aliases, and leaf-link deletion that removes the link rather than its target. Share the existing Windows handle primitive with catalog-cache I/O, add deletion and handle-lifetime coverage, and update the FileHelper contract.

Testing

Set up this checkout's .venv and editable install using the local development guide. Run commands from the checkout root.

• Tested locally with .venv/bin/specify --help (Windows PowerShell: .\.venv\Scripts\specify.exe --help)
• Ran relevant existing tests with .venv/bin/python -m pytest tests -q (Windows PowerShell: .\.venv\Scripts\python.exe -m pytest tests -q); use affected test paths for focused checks and LC_ALL=en_US.UTF-8 on macOS
• Tested with a sample project (if applicable)

.venv/bin/specify --help — passed.

Focused command:

.venv/bin/python -m pytest tests/test_file_helper.py tests/test_file_handles.py tests/specify_cli/workflows/test_catalog_cache.py tests/integrations/test_integration_state.py tests/specify_cli/test_file_policy.py tests/specify_cli/workflows/test_command_list.py tests/specify_cli/workflows/test_operation_list.py tests/test_presets.py -q

Result: 599 passed, 82 skipped on macOS. The full suite was not run. Native Windows handle-sharing behavior was not exercised; portable Windows handle-contract checks passed. Sample-project testing is not applicable to this filesystem-helper change.

AI Disclosure

• I did not use AI assistance for this contribution
• I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot, powered by GPT-6.1 Sol (gpt-6.1-sol), performed the implementation, test generation and execution, documentation changes, and commit/PR preparation on behalf of @mnriem in autonomous mode. The reasoning-effort setting was not exposed to the agent.

[Copilot]

🟡 Changes recommended

An ancestor-symlink race can redirect deletion outside the trusted root, while requiring read access to every directory also breaks deletion from write-and-execute-only parents.

2 open issues

• Pin the canonical root to prevent symlink replacement · New
• Avoid requiring read permission to unlink files · New

What changed in this PR

This PR moves FileHelper deletion to directory-handle-based access, aiming to prevent parent-link swaps while preserving existing symlink behavior.

Changes:

• Anchor recursive enumeration and removal using POSIX directory descriptors or Windows ancestor handles.
• Share the Windows handle primitive between FileHelper and catalog-cache I/O.
• Add deletion-race and handle-cleanup coverage, and document the safety guarantees and platform limits.

AI disclosure: GitHub Copilot performed autonomous review and focused checks for the requester; model and reasoning settings were unavailable.

File	Description
tests/​test_file_helper.py	Adds deletion-swap, descriptor-cleanup, Windows handle-lifetime, and root-alias tests.
tests/​test_file_handles.py	Adds portable Windows handle validation and cleanup checks.
src/​specify_cli/​workflows/​_catalog_cache.py	Imports the shared Windows handle primitive.
src/​specify_cli/​file_helper.py	Anchors recursive scans and removals to directory handles.
src/​specify_cli/​_file_handles.py	Extracts the reusable Windows handle context manager.
design/​file-helper.md	Defines handle-based deletion guarantees and remaining safety limits.

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

A critical POSIX symlink race can delete an unintended file because path validation uses a replacement tree while removal uses the original root.

1 open issue

• Resolve symlinks relative to the retained root descriptor · New

2 resolved since last review

• Pin the canonical root to prevent symlink replacement
• Avoid requiring read permission to unlink files

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

Deletion can target an attacker-replaced root after inode reuse, directory-open costs grow cubically with tree depth, and a permission test fails on POSIX hosts lacking both O_SEARCH and O_PATH.

1 open issue

• Carry an anchored directory descriptor through recursive deletion · New

2 previously missed issues

In code that hasn't changed since last review

Pin the root directory handle to prevent replacement

src/​specify_cli/​file_helper.py:280

If an attacker can remove and recreate the root between helper construction and delete(), inode reuse lets a replacement pass samestat, so deletion can operate on a different directory tree. Keep the established root pinned with a retained handle and explicit cleanup rather than relying solely on stored device and inode numbers.

Skip permission test when search-only flags are unavailable

tests/​test_file_helper.py:889

This test assumes deletion works through a write-and-execute-only directory, but _deletion_flags() intentionally falls back to O_RDONLY on POSIX hosts without O_SEARCH or O_PATH. On those hosts, the unreadable parent causes PermissionError, so the test fails despite matching the documented fallback. Skip this case when neither flag is available.

1 resolved since last review

• Resolve symlinks relative to the retained root descriptor

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

Outstanding regressions can block ordinary helper operations on Windows, cause deep-tree deletion to raise RecursionError, and leave root descriptors and Windows ancestor locks open on workflow error paths.

7 open issues

• Avoid requiring ancestor listing access for read-only helper operations · New
• Close project_files owner on every list_workflows exit · New
• Avoid recursive generator frames when planning deep directory deletion · New
• Test constructor failures at policy and integration-state boundaries · New
• Test helper cleanup in migrated policy and integration-state consumers · New
• Construct the helper before mocking os.open in overwrite error test · New
• Test recursive deletion cleanup on success and failure · New

1 resolved since last review

• Carry an anchored directory descriptor through recursive deletion

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

Recursive deletion now fails on previously supported deep trees, workflow error paths retain unclosed root handles, and macOS helpers reject previously supported write-and-search-only roots.

11 open issues

• Use Darwin search flags when anchoring the root · New
• Avoid recursive generator frames when planning deep directory deletion
• Close project_files owner on every list_workflows exit
• Avoid requiring ancestor listing access for read-only helper operations
• Document Windows ancestry access requirements · New
• Test deterministic cleanup in bootstrap consumers · New
• Test constructor failures and managed-state cleanup · New
• Test recursive deletion cleanup on success and failure
• Construct the helper before mocking os.open in overwrite error test
• Test helper cleanup in migrated policy and integration-state consumers
• Test constructor failures at policy and integration-state boundaries

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

A critical Windows root-acquisition race can retain a handle to the wrong directory, allowing subsequent deletions to target an unintended root.

8 open issues

• Verify the acquired Windows root handle matches the observed identity · New
• Raise a fresh copy of the cached Windows error on each delete · New
• Close project_files owner on every list_workflows exit
• Test constructor failures and managed-state cleanup
• Test deterministic cleanup in bootstrap consumers
• Construct the helper before mocking os.open in overwrite error test
• Test helper cleanup in migrated policy and integration-state consumers
• Test constructor failures at policy and integration-state boundaries

5 resolved since last review

• Use Darwin search flags when anchoring the root
• Avoid recursive generator frames when planning deep directory deletion
• Avoid requiring ancestor listing access for read-only helper operations
• Document Windows ancestry access requirements
• Test recursive deletion cleanup on success and failure

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

Windows error copying loses the native winerror and breaks the cached-capability-error test, while the deletion-replacement test fails during construction instead of exercising the intended deletion path.

8 open issues

• Preserve winerror when copying cached OSError instances · New
• Trigger the handle swap only during pinned acquisition · New
• Close project_files owner on every list_workflows exit
• Test constructor failures and managed-state cleanup
• Test deterministic cleanup in bootstrap consumers
• Construct the helper before mocking os.open in overwrite error test
• Test helper cleanup in migrated policy and integration-state consumers
• Test constructor failures at policy and integration-state boundaries

2 resolved since last review

• Verify the acquired Windows root handle matches the observed identity
• Raise a fresh copy of the cached Windows error on each delete

_{Review effort: Max (auto)}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate an unpublished recursive-deletion regression that can exhaust file descriptors on deeply nested directory trees.

No open issues 🎉

1 resolved since last review

• Bound descriptor use for deep-path deletion

_{Review effort: Max (auto)}

[Copilot]

🔵 Needs a closer look

Human assessment is needed because an unpublished compatibility finding indicates cached file-identity query failures can block every deletion on FAT32, exFAT, and some SMB volumes.

No open issues 🎉

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

On Windows, reparse-point changes after preflight can redirect pathname-based checks and deletions outside the root even when symlinks are disallowed.

2 open issues

• Anchor Windows deletion operations to native directory handles · New
• Install fstat patches after constructing the helper · New

_{Review effort: Max (auto)}

[Copilot]

🔵 Needs a closer look

Two fstat failure-injection tests now pass on constructor errors without reaching the read/overwrite checks, silently losing their intended descriptor-cleanup coverage.

2 open issues

• Anchor Windows deletion operations to native directory handles
• Install fstat patches after constructing the helper

_{Review effort: Max (auto)}

[Copilot]

🟡 Changes recommended

Recursive Windows deletion opens directory handles without the required synchronization flags, risking enumeration failures or buffer reads before I/O completion.

1 open issue

• Open recursive directory handles with synchronous I/O flags · New

2 resolved since last review

• Anchor Windows deletion operations to native directory handles
• Install fstat patches after constructing the helper

_{Review effort: Max (auto)}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate a Windows recursive-deletion failure involving delete-pending children with open share-delete handles.

No open issues 🎉

1 resolved since last review

• Open recursive directory handles with synchronous I/O flags

_{Review effort: Max (auto)}