Improve error handling in write_manifest test helper function

[coderabbitai]

The write_manifest function in tests/support/mod.rs (line 109) currently panics on write errors using .expect().

Current code:

.expect("write manifest content");

Improvement needed:

• Refactor function to return a Result type
• Propagate errors instead of panicking
• Enable clearer diagnostics when manifest writing fails

This change would provide better error handling and make test failures more debuggable.

Backlinks:

• Original PR: Handle manifest path in process tests #56
• Discussion: Handle manifest path in process tests #56 (comment)
• Issue: Improve error handling in write_manifest test helper function #62

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

@leynos I need to examine the current state of the write_manifest function to determine if this issue is still relevant. Let me check the codebase.

---

🏁 Script executed:

#!/bin/bash
# First, let's find the write_manifest function in the codebase
fd -t f -e rs | xargs rg -n "write_manifest" -A 10 -B 2

# Also check specifically in tests/support/mod.rs around line 109
if [ -f "tests/support/mod.rs" ]; then
    echo "=== Content around line 109 in tests/support/mod.rs ==="
    sed -n '100,120p' tests/support/mod.rs
fi

Length of output: 1624

---

Let me examine the current implementation more closely to understand the full scope:

---

🏁 Script executed:

#!/bin/bash
# Let's look at the complete write_manifest function implementation
echo "=== write_manifest function implementation ==="
rg -A 15 -B 5 "pub fn write_manifest" test_support/src/env.rs

echo -e "\n=== All usages of write_manifest in the codebase ==="
rg -n "write_manifest" --type rust -C 3

echo -e "\n=== Check if there are other expect() calls in test support ==="
rg "\.expect\(" test_support/src/ -n -C 2

Length of output: 6153

---

Now let me apply the appropriate GitHub label and provide my assessment:

@leynos Based on my analysis of the codebase, this issue is still relevant but has been partially addressed.

Current Status

Partially Fixed: The write_manifest function itself has been properly refactored:

• ✅ Now located in test_support/src/env.rs:167
• ✅ Returns io::Result<()> instead of panicking internally
• ❌ However, the caller in test_support/src/lib.rs:142 still uses .expect(), which means tests still panic on write errors

// Current problematic code:
crate::env::write_manifest(&mut file).expect(&format!(
    "Failed to write manifest content to {}",
    manifest_path.display()
));

Criticality Assessment: MEDIUM

Risk Analysis:

• User Impact: Low (affects only test infrastructure, not production)
• Blast Radius: Medium (affects developer experience and test debugging)
• Developer Impact: Medium (makes test failures harder to diagnose)

Required Changes Impact

Scope: The remaining work involves refactoring the caller in test_support/src/lib.rs to:

1. Change the ensure_manifest_exists function to return Result<PathBuf, io::Error>
2. Propagate the write_manifest error instead of panicking
3. Update all call sites of ensure_manifest_exists to handle the Result

Note: I found multiple other .expect() calls in the test support code that follow the same pattern and could benefit from similar treatment for consistency.

The issue remains valid and would improve test reliability and debugging experience when manifest operations fail.

---

🏁 Script executed:

#!/bin/bash
# Apply the medium criticality label to the issue
gh issue edit 62 --add-label "medium"

Length of output: 81

---

Perfect! I've successfully applied the medium criticality label to issue #62.

The analysis shows that while the core write_manifest function has been improved to return Result<()>, the calling code still uses .expect() which maintains the problematic panic behavior. This issue deserves medium priority as it impacts test reliability and debugging capabilities for developers, though it doesn't affect production users directly.