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

[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.