cleveragents/cleveragents-core

Fork 3

refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp() #730

New issue

Open

opened 2026-03-12 13:18:29 +00:00 by CoreRasurae · 3 comments

CoreRasurae commented

2026-03-12 13:18:29 +00:00

Member

Metadata

Commit Message: refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()
Branch: refactor/m3-replace-mktemp

Summary

The test infrastructure uses tempfile.mktemp() across 16+ call sites. This function has been deprecated since Python 2.3 due to a TOCTOU race condition: between name generation and file creation, another process could place a symlink at the predicted path. Under 16-worker parallel test execution, there is also a non-zero risk of path collisions.

Background and Context

Identified during the code review of PR #727 (Finding #7 in the APPROVED review, Security Review Finding #2, and Performance Review P3 by @hurui200320). These are pre-existing issues not introduced by that PR.

Current Behavior

tempfile.mktemp() is used at approximately 16 call sites:

features/environment.py:48, 52, 292
~13 other step files

The function generates a temporary file name without atomically creating the file, leaving a window where another process could create a file at that path.

Expected Behavior

All tempfile.mktemp() call sites should be replaced with tempfile.mkstemp() (which atomically creates the file and returns an fd + path) followed by os.close(fd) to release the file descriptor. For cases where the path is used for SQLite template copying, the pattern is:

fd, db_path = tempfile.mkstemp(suffix='.db', prefix=prefix)
os.close(fd)
# Now use db_path — the file already exists atomically

Alternatively, tempfile.NamedTemporaryFile(delete=False, suffix='.db', prefix=prefix) can be used where appropriate.

Acceptance Criteria

Zero remaining uses of tempfile.mktemp() in the codebase
All replacements use tempfile.mkstemp() + os.close(fd) or equivalent atomic approach
All tests pass under both sequential and parallel (16-worker) execution
No Python deprecation warnings related to tempfile

Supporting Information

Identified in: PR #727 Code Review Finding #7, Security Review Finding #2, Performance Review P3
Review URL: #727
Reviewer: @hurui200320
Python docs: https://docs.python.org/3/library/tempfile.html#tempfile.mktemp

Subtasks

Identify all tempfile.mktemp() call sites (grep across features/ directory)
Replace each call site with tempfile.mkstemp() + os.close(fd) pattern
Verify template-copy flow still works (mkstemp creates the file, so copy2/copyfile overwrites it)
Run nox -s unit_tests with --processes 1 (sequential)
Run nox -s unit_tests with --processes 16 (parallel)

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

## Metadata - **Commit Message**: `refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()` - **Branch**: `refactor/m3-replace-mktemp` ## Summary The test infrastructure uses `tempfile.mktemp()` across 16+ call sites. This function has been [deprecated since Python 2.3](https://docs.python.org/3/library/tempfile.html#tempfile.mktemp) due to a TOCTOU race condition: between name generation and file creation, another process could place a symlink at the predicted path. Under 16-worker parallel test execution, there is also a non-zero risk of path collisions. ## Background and Context Identified during the code review of PR #727 (Finding #7 in the APPROVED review, Security Review Finding #2, and Performance Review P3 by @hurui200320). These are pre-existing issues not introduced by that PR. ## Current Behavior `tempfile.mktemp()` is used at approximately 16 call sites: - `features/environment.py:48, 52, 292` - ~13 other step files The function generates a temporary file name without atomically creating the file, leaving a window where another process could create a file at that path. ## Expected Behavior All `tempfile.mktemp()` call sites should be replaced with `tempfile.mkstemp()` (which atomically creates the file and returns an fd + path) followed by `os.close(fd)` to release the file descriptor. For cases where the path is used for SQLite template copying, the pattern is: ```python fd, db_path = tempfile.mkstemp(suffix='.db', prefix=prefix) os.close(fd) # Now use db_path — the file already exists atomically ``` Alternatively, `tempfile.NamedTemporaryFile(delete=False, suffix='.db', prefix=prefix)` can be used where appropriate. ## Acceptance Criteria - [ ] Zero remaining uses of `tempfile.mktemp()` in the codebase - [ ] All replacements use `tempfile.mkstemp()` + `os.close(fd)` or equivalent atomic approach - [ ] All tests pass under both sequential and parallel (16-worker) execution - [ ] No Python deprecation warnings related to tempfile ## Supporting Information - Identified in: PR #727 Code Review Finding #7, Security Review Finding #2, Performance Review P3 - Review URL: https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/727 - Reviewer: @hurui200320 - Python docs: https://docs.python.org/3/library/tempfile.html#tempfile.mktemp ## Subtasks - [ ] Identify all `tempfile.mktemp()` call sites (grep across features/ directory) - [ ] Replace each call site with `tempfile.mkstemp()` + `os.close(fd)` pattern - [ ] Verify template-copy flow still works (mkstemp creates the file, so copy2/copyfile overwrites it) - [ ] Run `nox -s unit_tests` with `--processes 1` (sequential) - [ ] Run `nox -s unit_tests` with `--processes 16` (parallel) ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done.

CoreRasurae added the

labels

2026-03-12 13:20:24 +00:00

CoreRasurae referenced this issue from a commit

2026-03-12 14:29:08 +00:00

refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()

CoreRasurae referenced this issue from a pull request that will close it,

2026-03-12 14:29:35 +00:00

refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp() #736

CoreRasurae added

and removed

labels

2026-03-12 14:30:03 +00:00

CoreRasurae commented

2026-03-12 14:31:54 +00:00

Author

Member

Implementation submitted as PR #736.

All 28 tempfile.mktemp() call sites replaced with atomic tempfile.mkstemp() + os.close(fd) across 16 files. All 10,640 BDD scenarios pass with no regressions.

Implementation submitted as PR #736. All 28 `tempfile.mktemp()` call sites replaced with atomic `tempfile.mkstemp()` + `os.close(fd)` across 16 files. All 10,640 BDD scenarios pass with no regressions.

freemo added the

Points

label

2026-03-12 20:57:22 +00:00

freemo added this to the v3.2.0 milestone

2026-03-12 21:01:14 +00:00

CoreRasurae was assigned by freemo

2026-03-12 21:01:30 +00:00

freemo commented

2026-03-12 21:04:43 +00:00

Owner

PM Acknowledgment — Day 32

Confirmed. PR #736 has been reviewed and APPROVED (Review ID 2170). All 28 mktemp() replacements verified, clean tempfile fix.

This issue is now assigned to @CoreRasurae with milestone v3.2.0 (M3). PR #736 is in the merge queue — awaiting @freemo to merge.

Status: State/In Review → awaiting merge.

**PM Acknowledgment — Day 32** Confirmed. PR #736 has been reviewed and **APPROVED** (Review ID 2170). All 28 `mktemp()` replacements verified, clean tempfile fix. This issue is now assigned to @CoreRasurae with milestone v3.2.0 (M3). PR #736 is in the merge queue — awaiting @freemo to merge. **Status**: State/In Review → awaiting merge.

freemo commented

2026-03-16 10:24:53 +00:00

Owner

PM Status — Day 36 (2026-03-16)

Status: PR #736 approved (Day 32). However, a rebase was requested on Day 34 (PR has merge conflicts). @CoreRasurae — please rebase PR #736 and it can be merged immediately. This is a quick win.

## PM Status — Day 36 (2026-03-16) **Status**: PR #736 approved (Day 32). However, a rebase was requested on Day 34 (PR has merge conflicts). @CoreRasurae — please rebase PR #736 and it can be merged immediately. This is a quick win.

CoreRasurae was unassigned by freemo

2026-04-02 06:13:51 +00:00

freemo self-assigned this

2026-04-02 06:13:51 +00:00

freemo referenced this issue

2026-04-02 17:36:27 +00:00

refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp() #736

freemo removed the