Skip to content

Use handle-based text overwrites in FileHelper - #5353

Merged
mnriem merged 14 commits into
github:mainfrom
mnriem:mnriem-file-helper-overwrite-handles
Nov 10, 2026
Merged

mnriem merged 14 commits into
github:mainfrom
mnriem:mnriem-file-helper-overwrite-handles

Conversation

@mnriem

@mnriem mnriem commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Description

Use anchored directories and opened file descriptors for FileHelper.overwrite_text() while retaining its non-atomic text-upsert contract. Preserve supported contained symlink chains, trusted root aliases, existing inode and hard-link identity, permissions, and native text encoding and newline behavior.

Extract the existing Windows handle primitive into a shared private module, add overwrite coverage, and document the operation's filesystem behavior and limitations. This is an independent change targeting main, not a stacked PR.

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)

Commands and results:

  • .venv/bin/specify --help — passed.
  • LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests/test_file_helper.py tests/test_file_helper_overwrite.py tests/test_file_handles.py tests/specify_cli/workflows/test_catalog_cache.py tests/specify_cli/test_file_policy.py tests/integrations/test_integration_state.py -q — 489 passed, 88 skipped.
  • git diff --check — passed.

Validation ran on macOS. Native Windows tests are included but were not executed on this host. A separate sample-project run is not applicable; operation tests use temporary project roots. The full test suite was not run.

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 using GPT-6.1 Sol (model ID: gpt-6.1-sol) in autonomous mode with runtime-default reasoning settings authored the implementation, tests, documentation, commit message, and PR description, and ran local validation on behalf of @mnriem.

Anchor resolved overwrite destinations while retaining in-place updates, supported contained links, and native text behavior. Reuse shared Windows handle primitives and add overwrite regression coverage and contract documentation.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 23:05

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.

🟡 Changes recommended

The file-opening changes bypass Windows creation masks, reject otherwise valid writes beneath unreadable parent directories, and break existing Windows permission-failure tests.

3 open issues
What changed in this PR

This PR moves FileHelper.overwrite_text() to handle-based I/O to resist link-based path swaps while retaining non-atomic, in-place text updates.

Changes:

  • Anchor directory traversal and validate the opened file before truncation.
  • Share Windows handle utilities with workflow catalog caching.
  • Add overwrite and handle tests, and document filesystem guarantees and limitations.

Validation here used isolated POSIX checks; native Windows behavior was not executed.

File Description
tests/​test_file_helper_overwrite.py Covers path swaps, text behavior, metadata preservation, and cleanup.
tests/​test_file_handles.py Checks Windows handle flags, validation, and ownership.
src/​specify_cli/​workflows/​_catalog_cache.py Reuses the extracted Windows handle helper.
src/​specify_cli/​file_helper.py Implements anchored, descriptor-based text overwrites.
src/​specify_cli/​_file_handles.py Provides shared Windows handle and overwrite-descriptor primitives.
design/​file-helper.md Documents overwrite behavior, platform requirements, and safety limits.

Review effort: Max (auto)

Comment thread src/specify_cli/_file_handles.py Resolved
Comment thread src/specify_cli/file_helper.py Resolved
Comment thread src/specify_cli/file_helper.py Resolved
Honor the Windows creation mask, retain access through searchable unreadable POSIX directories, and inject overwrite failures at the shared descriptor boundary. Cover native creation-mask parity and directory permission success and failure cases.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 9, 2026 23:31
mnriem and others added 2 commits November 9, 2026 17:42
Use the Darwin kernel search bit when Python does not expose O_SEARCH or O_PATH, and exercise the missing-constant branch for existing and new overwrite destinations.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Compare native creation with the overwrite API's UTF-8 default explicitly, including on Python versions whose Windows locale encoding differs.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

🟡 Changes recommended

Concurrent Windows overwrites can corrupt the process-wide umask, POSIX traversal can exhaust file descriptors, and the Windows regression test can fail because it compares different encodings.

3 open issues
2 previously missed issues

In code that hasn't changed since last review

Medium severity Close previous POSIX directory FDs during traversal

src/​specify_cli/​file_helper.py:267

Keeping every POSIX ancestor in the ExitStack makes descriptor usage grow with path depth, although only the current directory FD is needed for anchored traversal. With 80 valid parent directories and RLIMIT_NOFILE=64, native text overwrite succeeds but this implementation raises EMFILE before opening the leaf. Please close each previous parent after opening its child, as _cache_directory already does; retain the full hierarchy only on Windows, where the sharing locks require it.

Low severity Remove the blanket overwrite_text sandbox exception

design/​file-helper.md:227

The blanket overwrite_text exception implies a race-free sandbox and excludes its writes from the partial-results warning. That conflicts with its write-failure contract at lines 180–182 and namespace limits at lines 242–246. Remove this exception and keep the narrower anchored-traversal guarantee in the operation's description above.

2 resolved since last review

Review effort: Max (auto)

Comment thread src/specify_cli/_file_handles.py Resolved
Comment thread tests/test_file_helper_overwrite.py Resolved
Copilot AI auto review requested due to automatic review settings November 9, 2026 23:53
Let exclusive CRT creation apply Windows creation masks while retaining anchored existing-file opens. Close previous POSIX directory descriptors during traversal and clarify overwrite limitations. Cover interleaved calls, deep paths under low descriptor limits, and Windows creation collisions.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

🔵 Needs a closer look

Overlapping overwrites can corrupt the Windows runtime permission mask, while directory pins unnecessarily require listing access and block otherwise permitted writes.

2 open issues
2 previously missed issues

In code that hasn't changed since last review

High severity Overlapping overwrites corrupt the process creation mask

src/​specify_cli/​_file_handles.py:46

Two overlapping overwrites can restore 0o200 over an original mask of 0, making a newly created file read-only and changing permissions for later file creations. Unrelated native text-file creation can also observe the temporary mask between these calls. Use a creation strategy that applies the Windows runtime's mask without mutating it, and add an overlapping-call regression; a helper-only lock would not protect unrelated file creators.

Medium severity Directory pins incorrectly require list access

src/​specify_cli/​file_helper.py:239

The reused _windows_handle(..., directory=True) requests FILE_LIST_DIRECTORY for every directory it pins. On Windows, a directory can allow traversal and writes to a known child while denying directory listing, so this now raises PermissionError for files that native Path.write_text() and the previous implementation can overwrite. Please use directory pins that require traversal rather than listing access while retaining rename protection, and add a native Windows ACL parity regression.

1 resolved since last review

Review effort: Max (auto)

Copilot AI auto review requested due to automatic review settings November 10, 2026 00:13
Capture masked Windows creation attributes with an empty private probe in the pinned parent, then exclusively create the destination through a no-follow native handle. Clean up the probe, preserve existing in-place updates, and cover leaf collisions during attribute capture.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

🟡 Changes recommended

A critical Windows symlink race can redirect exclusive file creation outside the helper root, violating the no-redirection guarantee.

2 open issues
1 resolved since last review

Review effort: Max (auto)

Comment thread src/specify_cli/_file_handles.py Resolved
Copilot AI auto review requested due to automatic review settings November 10, 2026 00:36

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.

🟡 Changes recommended

The Windows permission probe can fail when creation and writing are allowed but deletion is denied, leaving a probe file behind and aborting otherwise permitted destination creation.

2 open issues
1 resolved since last review

Review effort: Max (auto)

Comment thread src/specify_cli/_file_handles.py Resolved
Discover CRT-masked creation attributes in independent runtime-owned temporary storage instead of a disposable project sibling. Preserve native no-follow destination creation and cover create/write-without-delete directory ACLs.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 13:54
Resolve shared handle and filesystem contract conflicts while retaining the upstream exclusive-creation implementation and both test suites.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Nov 10, 2026
Apply create/write-without-delete permissions to the child destination directory, keeping root canonicalization separate from the overwrite permission regression.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

🔵 Needs a closer look

Human assessment is needed because Windows directory pinning can reject otherwise permitted text writes when ACLs deny directory listing.

1 open issue
1 resolved since last review

Review effort: Max (auto)

Copilot AI auto review requested due to automatic review settings November 10, 2026 14:16
Deny the advanced DE and DC rights explicitly and confirm ordinary file reading remains available before exercising overwrite behavior.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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.

🟡 Changes recommended

The overwrite documentation omits Windows pinning’s directory-listing and attribute-read requirements on the canonical root and resolved parents, leaving unexplained why restrictive access rules can prevent overwriting an otherwise writable file.

2 open issues

Review effort: Max (auto)

Comment thread design/file-helper.md Resolved
Copilot AI auto review requested due to automatic review settings November 10, 2026 14:38

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.

🔵 Needs a closer look

The overwrite contract omits the new Windows directory-list access requirement on the root and parent directories, leaving a platform-specific failure mode undocumented.

2 open issues

Review effort: Max (auto)

State that the canonical root and every resolved parent require directory-list and read-attributes access, including the resulting failure when an ACL allows file writes but denies listing.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 15:26

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.

🔵 Needs a closer look

Human assessment is needed because the FIFO test can pass without O_NONBLOCK, leaving the no-reader case vulnerable to an undetected indefinite hang.

1 open issue
1 resolved since last review

Review effort: Max (auto)

Exercise leaf replacement with a no-reader FIFO in timeout-bounded subprocesses for both link policies. Require the nonblocking ENXIO failure and preserve the original file contents while retaining reader-present file-type coverage.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 16: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.

🔵 Needs a closer look

Human review is needed to assess test coverage gaps around unsupported platform primitives, concurrent file creation, and creation-mask probe avoidance during Windows overwrites.

1 open issue

Review effort: Max (auto)

Exercise missing and zero-valued primitives, regular files introduced before POSIX open, and Windows existing-file probe avoidance. Check explicit failures and preserve inode, hard-link, and permission behavior.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 16:35

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.

🔵 Needs a closer look

Human assessment is needed to judge whether the concurrency test’s simulated umask state and timed waits introduce unnecessary complexity.

1 open issue

Review effort: Max (auto)

Reuse the direct process-mask assertion while retaining concurrent calls and descriptor cleanup checks. Remove simulated mask state, synchronization events, and timed interleaving waits.

Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI auto review requested due to automatic review settings November 10, 2026 17:43
@mnriem

mnriem commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator Author

Updated in 032dad0d: the concurrency test now runs two overwrite calls and relies on the existing assertion that os.umask must not be called, with no simulated mask state or timed waits. It still checks descriptor cleanup. .venv/bin/python -m pytest tests/test_file_handles.py tests/test_file_helper.py::test_overwrite_text_io_errors_propagate -q passed (38 tests). GitHub Copilot (GPT-6.1 Sol, gpt-6.1-sol, autonomous; reasoning setting not exposed) generated this test change, ran validation, and wrote this comment on behalf of @mnriem.

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.

🔵 Needs a closer look

The new Win32/CRT filesystem paths lack native Windows validation, and macOS-only runs with portable mocks cannot establish sharing, ACL, creation-mask, or handle-ownership behavior.

No open issues 🎉

1 resolved since last review

Review effort: Max (auto)

@mnriem
mnriem merged commit 7ec720e into github:main Nov 10, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants