Replace NamedTempFile::persist with more explicit file creation method

[coderabbitai]

The current implementation in tests/steps/process_steps.rs (line 70) uses NamedTempFile::persist() which overwrites existing files without explicit control.

Current code:

file.persist(&manifest_path)
    .expect("Failed to persist manifest file");

Suggested alternatives:

• Use fs::write for explicit file writing
• Use NamedTempFile::persist_noclobber to avoid accidental overwrites

This change would provide more explicit control over file creation behavior and prevent unintended file overwrites.

Backlinks:

• Original PR: Handle manifest path in process tests #56
• Discussion: Handle manifest path in process tests #56 (comment)
• Issue: Replace NamedTempFile::persist with more explicit file creation method #61

Requested by: @leynos

Rescope (2026-08-12)

The originally cited file tests/steps/process_steps.rs no longer exists. The sole remaining NamedTempFile::persist call is test_support/src/manifest.rs:103, which already treats ErrorKind::AlreadyExists as success rather than clobbering, so the original overwrite hazard is substantially mitigated. Remaining scope: adopt explicit no-clobber semantics at that call site, which is what PR #545 delivers. This issue closes with #545.

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.

Once a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's find the specific file mentioned in the issue
fd "process_steps.rs" --type f

# Look for any NamedTempFile usage in the codebase
rg -n "NamedTempFile" --type rust

# Specifically search for persist usage
rg -n "\.persist" --type rust

# Check if the specific line mentioned in the issue still exists
if [ -f "tests/steps/process_steps.rs" ]; then
    echo "=== Content around line 70 in tests/steps/process_steps.rs ==="
    sed -n '65,75p' tests/steps/process_steps.rs
    echo ""
    echo "=== Full context of persist usage ==="
    rg -n -A5 -B5 "persist" tests/steps/process_steps.rs || echo "No persist usage found in process_steps.rs"
fi

Length of output: 1557

---

🏁 Script executed:

#!/bin/bash
# Let's examine the actual persist usage in test_support/src/lib.rs
echo "=== Context around persist usage in test_support/src/lib.rs ==="
rg -n -A10 -B10 "persist" test_support/src/lib.rs

echo ""
echo "=== Full function context ==="
# Let's get more context to understand the function this persist is in
sed -n '130,160p' test_support/src/lib.rs

Length of output: 2679

---

🏁 Script executed:

#!/bin/bash
# Let's get the full function definition to understand the complete context
echo "=== Full function definition ==="
# Find the function that contains the persist call
ast-grep --pattern 'fn $FUNC($_) {
  $$$
  file.persist($_) $_
  $$$
}'

# Alternative approach - get more lines around the function
echo ""
echo "=== Broader context around persist usage ==="
sed -n '110,155p' test_support/src/lib.rs

# Let's also check what imports are used
echo ""
echo "=== Imports in test_support/src/lib.rs ==="
head -30 test_support/src/lib.rs | rg "use "

Length of output: 2172

---

🏁 Script executed:

#!/bin/bash
# Now let's apply the appropriate GitHub label and assess the issue
# First, let's check what labels are available
gh issue view 61 --json labels,title,body

# Apply the appropriate criticality label based on analysis
gh issue edit 61 --add-label "low"

echo "Label applied successfully"

Length of output: 1064

---

@leynos Looking at the codebase, the issue is still relevant but with important context differences from the original description.

Current Status

The NamedTempFile::persist() usage exists, but not where originally described:

• Location: test_support/src/lib.rs line 146 (not tests/steps/process_steps.rs line 70)
• Function: ensure_manifest_exists() in test support code

Current Implementation Analysis

The code is actually safer than the original issue suggests:

pub fn ensure_manifest_exists(temp_dir: &Path, cli_file: &Path) -> PathBuf {
    // ... path resolution logic ...
    
    if !manifest_path.exists() {  // ← Checks if file exists first
        // ... directory creation ...
        let mut file = NamedTempFile::new_in(dest_dir).expect(...);
        crate::env::write_manifest(&mut file).expect(...);
        file.persist(&manifest_path).expect(...);  // ← Only runs if file doesn't exist
    }
    manifest_path
}

Criticality Assessment: LOW

Rationale:

• Limited blast radius: Test support code only, not production
• Existing safety check: Only creates files when they don't exist
• Controlled environment: Used in test scenarios with predictable paths
• No user impact: Test infrastructure issue

Required Changes Impact

Code changes needed:

• Replace file.persist(&manifest_path) with file.persist_noclobber(&manifest_path) for explicit intent
• Or use fs::write() for more direct file creation
• Update error handling to account for potential file conflicts

Documentation:

• The issue description needs updating to reflect the correct file location
• Function documentation already mentions the persist behavior

The change would improve code clarity and defensive programming practices but poses minimal risk to the project.

I've applied the low criticality label to the issue.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

Replace NamedTempFile::persist with NamedTempFile::persist_noclobber in persist_manifest_file to express no-clobber intent explicitly and prevent unintended overwrites.

Preserve the current atomic-rename staging pattern and the tolerance for a concurrently created target file, so the single caller and existing unit tests remain compatible.

Add a regression test for the previously uncovered "file already exists" branch.

Make no dependency changes; persist_noclobber is available at the pinned tempfile version.

Design Choices

Design Choice 1: Which persistence method replaces the current `persist` call?

Options Considered:

1. Use NamedTempFile::persist_noclobber. This keeps the atomic staging-then-rename pattern and refuses to overwrite an existing target. It maps directly onto the current no-clobber intent.
2. Use test_support::fs::write to write default content straight to the resolved path. This is simpler but loses atomicity and would need a separate existence check to avoid clobbering.

Chosen Option: 1

Rationale: The ticket offers two alternatives: fs::write and persist_noclobber. These have different guarantees. persist_noclobber preserves the existing atomic-rename behaviour, expresses the no-clobber intent directly in the API, and requires no dependency changes.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Adopt explicit no-clobber persistence

Replace the implicit overwrite behaviour of persist with the explicit persist_noclobber API. Preserve the existing tolerance for a concurrently created target file. Add regression coverage for the previously untested "file already exists" path.

Task 1: Switch `persist_manifest_file` to `persist_noclobber`

Change the persistence call to refuse overwrites explicitly while keeping current success semantics.

• Update persist_manifest_file in test_support/src/manifest.rs (lines 106–120) to call file.persist_noclobber(manifest_path.as_std_path()) instead of file.persist(...).
• Keep the existing match structure: return Ok(()) on success, return Ok(()) when e.error.kind() == io::ErrorKind::AlreadyExists, and wrap any other error with the current contextual message that includes e.file.path().display() and e.error.
• Do not change the function signature or return type. The caller prepare_cli_with_directory in tests/bdd/steps/process.rs and the three colocated unit tests must keep compiling and passing.
• Update the doc comment for persist_manifest_file and any related comment to state that the function does not overwrite an existing manifest and tolerates a concurrently created target.

Task 2: Add regression coverage for the pre-existing target path

Add a unit test that drives the AlreadyExists branch inside persist_manifest_file, which currently has no coverage.

• Add a test to the #[cfg(test)] mod tests block in test_support/src/manifest.rs (lines 192–275).
• Pre-create a manifest file with known distinct content at the resolved target path before invoking the persistence path, so that persist_noclobber observes an existing target.
• Because ensure_manifest_exists short-circuits at its manifest_path.exists() check (line 52) when the file already exists, the test must reach persist_noclobber through the create path. Where the public entry point cannot reach that branch directly, construct the scenario at the helper level, or document why the branch is only reachable under a TOCTOU race and assert the tolerated outcome accordingly.
• Assert that the operation returns Ok, and that the pre-existing content remains unchanged. This confirms no-clobber behaviour.

🤖 Prompt for AI agents

Implement Phase 1: adopt explicit no-clobber persistence for manifest fixture
creation. Goal: replace `persist` with `persist_noclobber` in the manifest
helper and add missing regression coverage.

1. Edit `persist_manifest_file` in `test_support/src/manifest.rs` (lines
106–120):
- Replace the call `file.persist(manifest_path.as_std_path())` with
`file.persist_noclobber(manifest_path.as_std_path())`.
- Keep the match structure exactly as it is: `Ok(_) => Ok(())`; `Err(e) if
e.error.kind() == io::ErrorKind::AlreadyExists => Ok(())`; and the fallback
branch that wraps the error with a contextual message including
`e.file.path().display()` and `e.error`.
   - Do not alter the function signature or return type.
- Update the doc comment for `persist_manifest_file` to state that the function
does not overwrite an existing manifest and tolerates a concurrently created
target.

2. Confirm no other code needs changes: `prepare_cli_with_directory` in
`tests/bdd/steps/process.rs` calls `ensure_manifest_exists` only, so leave it
untouched. Do not touch `test_support/src/env.rs` (`write_manifest` is
unaffected). Do not bump the `tempfile` dependency in `Cargo.toml` or
`test_support/Cargo.toml`; version `3.8.0` (resolving to `3.27.0`) already
provides `persist_noclobber`.

3. Add one new unit test inside the `#[cfg(test)] mod tests` block in
`test_support/src/manifest.rs` (lines 192–275):
- Pre-create a manifest file with known, distinct content at the resolved target
path before the persistence path runs.
- Since `ensure_manifest_exists` short-circuits at its `manifest_path.exists()`
check (line 52) when the file already exists, drive the test through the create
path so `persist_noclobber` observes the pre-existing target. If the public
entry point cannot reach this branch directly, construct the scenario at the
helper level, or document clearly why the branch is only reachable under a
TOCTOU race, and assert the tolerated outcome accordingly.
- Assert the operation returns `Ok`, and assert the pre-existing content is
unchanged afterwards, confirming no-clobber behaviour.

4. Verify the three existing colocated unit tests for `ensure_manifest_exists`
still compile and pass, and run the `test_support` test suite plus the BDD steps
that call `ensure_manifest_exists` to confirm no regressions.

Research

The manifest fixture logic sits in test_support/src/manifest.rs. The public entry point ensure_manifest_exists resolves a manifest path inside a scenario temp directory. If the file is absent, it creates a NamedTempFile, writes default content through test_support::env::write_manifest, then persists the temp file onto the target path via the private helper persist_manifest_file. That helper currently calls file.persist(manifest_path.as_std_path()) and treats an io::ErrorKind::AlreadyExists error as success to tolerate a TOCTOU race. The single external caller is prepare_cli_with_directory in tests/bdd/steps/process.rs, which converts the io::Result into anyhow::Result with .context(...). Three unit tests colocated in manifest.rs exercise ensure_manifest_exists, but none exercise the AlreadyExists branch inside persist_manifest_file. The tempfile dependency is declared as 3.8.0 and resolves to 3.27.0; persist_noclobber is available without a version bump.

Ticket Summary: Replace the NamedTempFile::persist call with NamedTempFile::persist_noclobber to express explicit no-clobber semantics for manifest fixture creation. The ticket cites tests/steps/process_steps.rs line 70, but the actual persist call lives in test_support/src/manifest.rs inside persist_manifest_file; the process step file only calls into it.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Implement Phase 1: adopt explicit no-clobber persistence for manifest fixture
creation. Goal: replace `persist` with `persist_noclobber` in the manifest
helper and add missing regression coverage.

1. Edit `persist_manifest_file` in `test_support/src/manifest.rs` (lines
106–120):
- Replace the call `file.persist(manifest_path.as_std_path())` with
`file.persist_noclobber(manifest_path.as_std_path())`.
- Keep the match structure exactly as it is: `Ok(_) => Ok(())`; `Err(e) if
e.error.kind() == io::ErrorKind::AlreadyExists => Ok(())`; and the fallback
branch that wraps the error with a contextual message including
`e.file.path().display()` and `e.error`.
   - Do not alter the function signature or return type.
- Update the doc comment for `persist_manifest_file` to state that the function
does not overwrite an existing manifest and tolerates a concurrently created
target.

2. Confirm no other code needs changes: `prepare_cli_with_directory` in
`tests/bdd/steps/process.rs` calls `ensure_manifest_exists` only, so leave it
untouched. Do not touch `test_support/src/env.rs` (`write_manifest` is
unaffected). Do not bump the `tempfile` dependency in `Cargo.toml` or
`test_support/Cargo.toml`; version `3.8.0` (resolving to `3.27.0`) already
provides `persist_noclobber`.

3. Add one new unit test inside the `#[cfg(test)] mod tests` block in
`test_support/src/manifest.rs` (lines 192–275):
- Pre-create a manifest file with known, distinct content at the resolved target
path before the persistence path runs.
- Since `ensure_manifest_exists` short-circuits at its `manifest_path.exists()`
check (line 52) when the file already exists, drive the test through the create
path so `persist_noclobber` observes the pre-existing target. If the public
entry point cannot reach this branch directly, construct the scenario at the
helper level, or document clearly why the branch is only reachable under a
TOCTOU race, and assert the tolerated outcome accordingly.
- Assert the operation returns `Ok`, and assert the pre-existing content is
unchanged afterwards, confirming no-clobber behaviour.

4. Verify the three existing colocated unit tests for `ensure_manifest_exists`
still compile and pass, and run the `test_support` test suite plus the BDD steps
that call `ensure_manifest_exists` to confirm no regressions.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[leynos]

Rescoped (see the dated section appended to the description): the originally cited file tests/steps/process_steps.rs no longer exists, and the sole surviving persist call (test_support/src/manifest.rs:103) already treats AlreadyExists as success. Remaining scope is the explicit no-clobber semantics that PR #545 delivers; this issue should close with that PR.