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

Closed
CoreRasurae wants to merge 1 commit from refactor/m3-replace-mktemp into master
Member

Summary

Replaced all 28 uses of the deprecated tempfile.mktemp() function with the atomic tempfile.mkstemp() + os.close(fd) pattern across 16 test infrastructure and benchmark files. Added import os where it was not already present.

tempfile.mktemp() has been deprecated since Python 2.3 due to a TOCTOU (time-of-check-to-time-of-use) race condition: between name generation and file creation, another process could create a file at the predicted path. Under 16-worker parallel test execution (behave-parallel), there was also a non-zero risk of path collisions.

Closes #730

Motivation

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.

Changes

Files modified (28 call sites across 16 files + CHANGELOG):

File Call sites Change
features/environment.py 3 mktempmkstemp + os.close(fd)
robot/helper_resource_registry_migration.py 3 mktempmkstemp + os.close(fd), added import os
robot/helper_plan_phase_migration.py 1 mktempmkstemp + os.close(fd), added import os
robot/helper_plan_persistence_e2e.py 1 mktempmkstemp + os.close(fd), added import os
robot/helper_persistence_lifecycle.py 1 mktempmkstemp + os.close(fd), added import os
robot/helper_db_lifecycle_models.py 1 mktempmkstemp + os.close(fd), added import os
robot/helper_automation_profiles.py 1 mktempmkstemp + os.close(fd), added import os
features/steps/skill_discovery_steps.py 2 mktempmkstemp + os.close(fd), added import os
features/steps/resource_registry_tables_steps.py 2 mktempmkstemp + os.close(fd), added import os
features/steps/plan_persistence_steps.py 1 mktempmkstemp + os.close(fd), added import os
features/steps/persistence_robot_alignment_steps.py 1 mktempmkstemp + os.close(fd), added import os
features/steps/garbage_collection_cli_steps.py 3 mktempmkstemp + os.close(fd)
features/steps/decision_recording_steps.py 1 mktempmkstemp + os.close(fd)
features/steps/coverage_boost_steps.py 1 mktempmkstemp + os.close(fd)
features/steps/cli_streaming_steps.py 1 mktempmkstemp + os.close(fd)
features/steps/automation_profiles_guards_steps.py 1 mktempmkstemp + os.close(fd), added import os
benchmarks/persistence_robot_bench.py 4 mktempmkstemp + os.close(fd), added import os
CHANGELOG.md Added entry under Unreleased

Replacement pattern

# Before (deprecated, TOCTOU-vulnerable)
tmp = tempfile.mktemp(suffix=".db", prefix="prefix_")

# After (atomic file creation)
fd, tmp = tempfile.mkstemp(suffix=".db", prefix="prefix_")
os.close(fd)

Test Results

Metric Before After Delta
Features passed 374 374 Same
Scenarios passed 10,640 10,640 Same
Steps passed 40,632 40,632 Same
Behave total time 3m 51s 4m 01s +10s (noise)
Wall time 3m 27s 3m 36s +9s (noise)
Failures 0 0 Same

The ~9s wall-time delta (+4.3%) is within normal run-to-run variance for a 3.5-minute parallel test suite and does not represent a real regression.

Acceptance Criteria Verification

  • Zero remaining uses of tempfile.mktemp() in the codebase
  • All replacements use tempfile.mkstemp() + os.close(fd) atomic approach
  • All tests pass under parallel (16-worker) execution
  • No Python deprecation warnings related to tempfile
  • CHANGELOG.md updated
## Summary Replaced all 28 uses of the deprecated `tempfile.mktemp()` function with the atomic `tempfile.mkstemp()` + `os.close(fd)` pattern across 16 test infrastructure and benchmark files. Added `import os` where it was not already present. `tempfile.mktemp()` has been [deprecated since Python 2.3](https://docs.python.org/3/library/tempfile.html#tempfile.mktemp) due to a TOCTOU (time-of-check-to-time-of-use) race condition: between name generation and file creation, another process could create a file at the predicted path. Under 16-worker parallel test execution (`behave-parallel`), there was also a non-zero risk of path collisions. Closes #730 ## Motivation 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. ## Changes ### Files modified (28 call sites across 16 files + CHANGELOG): | File | Call sites | Change | |------|-----------|--------| | `features/environment.py` | 3 | `mktemp` → `mkstemp` + `os.close(fd)` | | `robot/helper_resource_registry_migration.py` | 3 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `robot/helper_plan_phase_migration.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `robot/helper_plan_persistence_e2e.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `robot/helper_persistence_lifecycle.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `robot/helper_db_lifecycle_models.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `robot/helper_automation_profiles.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `features/steps/skill_discovery_steps.py` | 2 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `features/steps/resource_registry_tables_steps.py` | 2 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `features/steps/plan_persistence_steps.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `features/steps/persistence_robot_alignment_steps.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `features/steps/garbage_collection_cli_steps.py` | 3 | `mktemp` → `mkstemp` + `os.close(fd)` | | `features/steps/decision_recording_steps.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)` | | `features/steps/coverage_boost_steps.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)` | | `features/steps/cli_streaming_steps.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)` | | `features/steps/automation_profiles_guards_steps.py` | 1 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `benchmarks/persistence_robot_bench.py` | 4 | `mktemp` → `mkstemp` + `os.close(fd)`, added `import os` | | `CHANGELOG.md` | — | Added entry under Unreleased | ### Replacement pattern ```python # Before (deprecated, TOCTOU-vulnerable) tmp = tempfile.mktemp(suffix=".db", prefix="prefix_") # After (atomic file creation) fd, tmp = tempfile.mkstemp(suffix=".db", prefix="prefix_") os.close(fd) ``` ## Test Results | Metric | Before | After | Delta | |--------|--------|-------|-------| | Features passed | 374 | 374 | Same | | Scenarios passed | 10,640 | 10,640 | Same | | Steps passed | 40,632 | 40,632 | Same | | Behave total time | 3m 51s | 4m 01s | +10s (noise) | | Wall time | 3m 27s | 3m 36s | +9s (noise) | | Failures | 0 | 0 | Same | The ~9s wall-time delta (+4.3%) is within normal run-to-run variance for a 3.5-minute parallel test suite and does not represent a real regression. ## Acceptance Criteria Verification - [x] Zero remaining uses of `tempfile.mktemp()` in the codebase - [x] All replacements use `tempfile.mkstemp()` + `os.close(fd)` atomic approach - [x] All tests pass under parallel (16-worker) execution - [x] No Python deprecation warnings related to tempfile - [x] CHANGELOG.md updated
refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
CI / quality (pull_request) Successful in 17s
CI / build (pull_request) Successful in 25s
CI / typecheck (pull_request) Successful in 38s
CI / security (pull_request) Successful in 43s
CI / unit_tests (pull_request) Successful in 3m7s
CI / docker (pull_request) Successful in 41s
CI / integration_tests (pull_request) Successful in 5m3s
CI / coverage (pull_request) Successful in 5m26s
CI / benchmark-regression (pull_request) Successful in 38m33s
532f128a49
Replaced all 28 uses of the deprecated tempfile.mktemp() function with the
atomic tempfile.mkstemp() + os.close(fd) pattern across 16 test infrastructure
and benchmark files.  Added import os where it was not already present.

tempfile.mktemp() has been deprecated since Python 2.3 due to a TOCTOU race
condition: between name generation and file creation, another process could
create a file at the predicted path.  Under 16-worker parallel test execution,
there was also a non-zero risk of path collisions.

Files modified:
- features/environment.py (3 call sites)
- robot/helper_resource_registry_migration.py (3 call sites)
- robot/helper_plan_phase_migration.py (1 call site)
- robot/helper_plan_persistence_e2e.py (1 call site)
- robot/helper_persistence_lifecycle.py (1 call site)
- robot/helper_db_lifecycle_models.py (1 call site)
- robot/helper_automation_profiles.py (1 call site)
- features/steps/skill_discovery_steps.py (2 call sites)
- features/steps/resource_registry_tables_steps.py (2 call sites)
- features/steps/plan_persistence_steps.py (1 call site)
- features/steps/persistence_robot_alignment_steps.py (1 call site)
- features/steps/garbage_collection_cli_steps.py (3 call sites)
- features/steps/decision_recording_steps.py (1 call site)
- features/steps/coverage_boost_steps.py (1 call site)
- features/steps/cli_streaming_steps.py (1 call site)
- features/steps/automation_profiles_guards_steps.py (1 call site)
- benchmarks/persistence_robot_bench.py (4 call sites)

All 10,640 BDD scenarios pass.  No performance regression observed (within
normal run-to-run variance).  Zero remaining uses of tempfile.mktemp() in
the codebase.

ISSUES CLOSED: #730
freemo added this to the v3.2.0 milestone 2026-03-12 20:22:21 +00:00
freemo approved these changes 2026-03-12 20:36:52 +00:00
freemo left a comment

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

Diff Summary

  • Files changed: 18 (16 test/benchmark files + CHANGELOG.md + 1 duplicate import addition)
  • Lines: +82 / −32

Commit Message Compliance

  • Issue #730 metadata: refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()
  • PR commit first line: refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()
  • Match: Exact match

Closes Keyword

  • PR body contains Closes #730

Label Compliance

Required Label Status
Type label Type/Task
Priority label Missing on PR (issue has Priority/Medium)
MoSCoW label Missing on both PR and issue #730

Code Quality

  • Every replacement follows the identical, correct pattern: fd, tmp = tempfile.mkstemp(...)os.close(fd). This eliminates the TOCTOU race condition from the deprecated mktemp().
  • import os is added exactly where it was previously absent — no unnecessary import additions.
  • CHANGELOG.md is updated with a clear, well-written entry.
  • All 10,640 BDD scenarios reported passing under both sequential and 16-worker parallel execution.
  • The one area I'd note for future reference: features/steps/resource_registry_tables_steps.py has a duplicate import os and import tempfile inside the function step_run_init_database_twice (line ~958) — these are local re-imports, harmless but slightly inconsistent with other call sites that use module-level imports. Not a blocker.

Verdict: APPROVED

Clean, mechanical, consistent refactoring with no behavioral changes. Well-documented PR body with test metrics. Minor note: please add Priority/Medium label to the PR, and consider adding a MoSCoW label to both issue #730 and this PR.

## Review — PR #736: refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp() ### Diff Summary - **Files changed:** 18 (16 test/benchmark files + CHANGELOG.md + 1 duplicate import addition) - **Lines:** +82 / −32 ### Commit Message Compliance - **Issue #730 metadata:** `refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()` - **PR commit first line:** `refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()` - **Match:** ✅ Exact match ### Closes Keyword - PR body contains `Closes #730` ✅ ### Label Compliance | Required Label | Status | |---|---| | Type label | ✅ `Type/Task` | | Priority label | ❌ Missing on PR (issue has `Priority/Medium`) | | MoSCoW label | ❌ Missing on both PR and issue #730 | ### Code Quality - Every replacement follows the identical, correct pattern: `fd, tmp = tempfile.mkstemp(...)` → `os.close(fd)`. This eliminates the TOCTOU race condition from the deprecated `mktemp()`. - `import os` is added exactly where it was previously absent — no unnecessary import additions. - CHANGELOG.md is updated with a clear, well-written entry. - All 10,640 BDD scenarios reported passing under both sequential and 16-worker parallel execution. - The one area I'd note for future reference: `features/steps/resource_registry_tables_steps.py` has a duplicate `import os` and `import tempfile` inside the function `step_run_init_database_twice` (line ~958) — these are local re-imports, harmless but slightly inconsistent with other call sites that use module-level imports. Not a blocker. ### Verdict: APPROVED Clean, mechanical, consistent refactoring with no behavioral changes. Well-documented PR body with test metrics. Minor note: please add `Priority/Medium` label to the PR, and consider adding a MoSCoW label to both issue #730 and this PR.
Owner

Rebase Required

@CoreRasurae — This PR has merge conflicts with master. Please rebase onto the latest master and force-push. See also: #656, #804, #806, #807 (all need rebase).

## Rebase Required @CoreRasurae — This PR has merge conflicts with `master`. Please rebase onto the latest `master` and force-push. See also: #656, #804, #806, #807 (all need rebase).
Owner

PM Status Update — Day 34

This is a low-risk, well-documented refactor (replaces deprecated tempfile.mktemp() across 28 call sites in 16 files). All tests pass, zero regressions. Should be a quick merge after rebase.

@CoreRasurae — Rebase onto master and force-push. This is the safest of your 5 conflicted PRs and should be rebased first for a quick win.

Reviewer needed: @hurui200320 — please do a quick approval review after Luis rebases. This is straightforward.

Priority: Medium (M3, low risk, quick merge)

**PM Status Update — Day 34** This is a low-risk, well-documented refactor (replaces deprecated `tempfile.mktemp()` across 28 call sites in 16 files). All tests pass, zero regressions. Should be a quick merge after rebase. **@CoreRasurae** — Rebase onto master and force-push. This is the safest of your 5 conflicted PRs and should be rebased first for a quick win. **Reviewer needed:** @hurui200320 — please do a quick approval review after Luis rebases. This is straightforward. **Priority:** Medium (M3, low risk, quick merge)
Owner

PM Status — Day 36

Rebase still needed. This is the safest of the conflicted PRs — low-risk refactor replacing deprecated tempfile.mktemp() across 28 call sites.

@CoreRasurae — Rebase this PR onto master. It should be a quick merge after rebase. This can be done alongside your other rebase tasks (#804, #806).

@hurui200320 — Quick approval review after rebase.


PM status comment — Day 36

## PM Status — Day 36 **Rebase still needed.** This is the safest of the conflicted PRs — low-risk refactor replacing deprecated `tempfile.mktemp()` across 28 call sites. @CoreRasurae — Rebase this PR onto master. It should be a quick merge after rebase. This can be done alongside your other rebase tasks (#804, #806). @hurui200320 — Quick approval review after rebase. --- *PM status comment — Day 36*
freemo left a comment

PM Day 36 Triage: MERGE CONFLICT. @CoreRasurae please rebase onto master at your earliest convenience. This is a low-priority cleanup PR targeting v3.2.0 (M3). Reviewer: @brent.edwards — please review once rebased.

PM Day 36 Triage: MERGE CONFLICT. @CoreRasurae please rebase onto master at your earliest convenience. This is a low-priority cleanup PR targeting v3.2.0 (M3). Reviewer: @brent.edwards — please review once rebased.
freemo self-assigned this 2026-04-02 06:15:26 +00:00
Owner

🤖 Backlog Groomer (groomer-1): Closing as duplicate of #730.

Issue #730 (refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()) is the canonical version with full labels (MoSCoW/Should have, Priority/Medium, State/In Review, Type/Task) and milestone v3.2.0. This issue is an exact title duplicate.

🤖 **Backlog Groomer (groomer-1):** Closing as duplicate of #730. Issue #730 (`refactor(test): replace deprecated tempfile.mktemp() with tempfile.mkstemp()`) is the canonical version with full labels (`MoSCoW/Should have`, `Priority/Medium`, `State/In Review`, `Type/Task`) and milestone `v3.2.0`. This issue is an exact title duplicate.
freemo closed this pull request 2026-04-02 17:36:46 +00:00
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
Required
Details
CI / quality (pull_request) Successful in 17s
Required
Details
CI / build (pull_request) Successful in 25s
Required
Details
CI / typecheck (pull_request) Successful in 38s
Required
Details
CI / security (pull_request) Successful in 43s
Required
Details
CI / unit_tests (pull_request) Successful in 3m7s
Required
Details
CI / docker (pull_request) Successful in 41s
Required
Details
CI / integration_tests (pull_request) Successful in 5m3s
Required
Details
CI / coverage (pull_request) Successful in 5m26s
Required
Details
CI / benchmark-regression (pull_request) Successful in 38m33s

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!736
No description provided.