Handle manifest path in process tests #56

coderabbitai · 2025-08-05T23:04:34Z

⚠️ Potential issue

Replace NamedTempFile::persist() with safer alternatives.

Using NamedTempFile::persist() overwrites existing files, as noted in the PR objectives. Use fs::write or persist_noclobber for more explicit control.

- file.persist(&manifest_path) - .expect("Failed to persist manifest file"); + drop(file); + std::fs::write(&manifest_path, &manifest_content) + .expect("Failed to write manifest file");

Alternatively, use persist_noclobber:

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

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In tests/steps/process_steps.rs at lines 73 to 74, replace the use of NamedTempFile::persist() with a safer alternative to avoid overwriting existing files. Use either fs::write to write the file contents explicitly or use persist_noclobber to persist the file without overwriting. This change ensures more explicit control over file persistence and aligns with the PR objectives.

@coderabbitai Please create a new GitHub issue for this

🧩 Analysis chain

@leynos I've created a GitHub issue to track the NamedTempFile::persist() safety concern. The issue includes the technical details, suggested solutions, and references back to this PR and comment for proper context.

🏁 Script executed:

#!/bin/bash # Create a GitHub issue for the NamedTempFile::persist() safety concern gh issue create \ --title "Replace NamedTempFile::persist() with safer alternatives in process tests" \ --body "## Problem The current implementation in \`tests/steps/process_steps.rs\` uses \`NamedTempFile::persist()\` which overwrites existing files without explicit control. This poses a potential safety risk and doesn't align with best practices for file handling. ## Location - File: \`tests/steps/process_steps.rs\` - Lines: 73-74 (in the \`run()\` function) ## Suggested Solutions 1. **Use \`fs::write\`** for explicit file writing: \`\`\`rust drop(file); std::fs::write(&manifest_path, &manifest_content) .expect(\"Failed to write manifest file\"); \`\`\` 2. **Use \`persist_noclobber\`** to avoid overwriting: \`\`\`rust file.persist_noclobber(&manifest_path) .expect(\"Failed to persist manifest file\"); \`\`\` ## Context This issue was identified during code review of PR #56, where the focus was on improving manifest path handling in process tests. ## References - PR: https://github.com/leynos/netsuke/pull/56 - Original comment: https://github.com/leynos/netsuke/pull/56#discussion_r2255505420" \ --assignee leynos

Length of output: 1191

@leynos Perfect! I've successfully created GitHub issue #58 to track this improvement. The issue is available at: #58

The issue includes:

Clear problem description about the NamedTempFile::persist() safety concern

Specific file location and line numbers

Two alternative solutions (fs::write and persist_noclobber)

Context linking back to this PR and comment thread

Assignment to you for future implementation

This will help ensure the improvement doesn't get lost and can be addressed in a future PR.

🧬 Code Graph Analysis Results

Relevant snippets from tests/support/mod.rs (lines 17-30, 41-65, 147-159)

pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); let path = dir.path().join("ninja"); let mut file = File::create(&path).expect("script"); writeln!(file, "#!/bin/sh\nexit {exit_code}").expect("write script"); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; let mut perms = fs::metadata(&path).expect("meta").permissions(); perms.set_mode(0o755); fs::set_permissions(&path, perms).expect("perms"); } (dir, path) } pub fn fake_ninja_check_build_file() -> (TempDir, PathBuf) { let dir = TempDir::new().expect("temp dir"); let path = dir.path().join("ninja"); let mut file = File::create(&path).expect("script"); writeln!( file, concat!( "#!/bin/sh\n", "if [ \"$1\" = \"-f\" ] && [ ! -f \"$2\" ]; then\n", " echo 'missing build file: $2' >&2\n", " exit 1\n", "fi\n", "exit 0" ) ) .expect("write script"); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; let mut perms = fs::metadata(&path).expect("meta").permissions(); perms.set_mode(0o755); fs::set_permissions(&path, perms).expect("perms"); } (dir, path) } pub fn write_manifest(file: &mut impl Write) -> io::Result<()> { writeln!( file, concat!( "netsuke_version: \"1.0.0\"\n", "targets:\n", " - name: hello\n", " recipe:\n", " kind: command\n", " command: \"echo hi\"\n" ), ) }

Relevant snippet from src/runner.rs (lines 236-309)

pub fn run_ninja( program: &Path, cli: &Cli, build_file: &Path, targets: &BuildTargets, ) -> io::Result<()> { let mut cmd = Command::new(program); if let Some(dir) = &cli.directory { // Resolve and canonicalise the directory so Ninja receives a stable // absolute path. Using only `current_dir` avoids combining it with // Ninja's own `-C` flag which would otherwise double-apply the // directory and break relative paths. let dir = fs::canonicalize(dir)?; cmd.current_dir(dir); } if let Some(jobs) = cli.jobs { cmd.arg("-j").arg(jobs.to_string()); } // Canonicalise the build file path so Ninja resolves it correctly from the // working directory. Fall back to the original on failure so Ninja can // surface a meaningful error. let build_file_path = build_file .canonicalize() .unwrap_or_else(|_| build_file.to_path_buf()); cmd.arg("-f").arg(&build_file_path); cmd.args(targets.as_slice()); cmd.stdout(Stdio::piped()); cmd.stderr(Stdio::piped()); let program = cmd.get_program().to_string_lossy().into_owned(); let args: Vec<CommandArg> = cmd .get_args() .map(|a| CommandArg::new(a.to_string_lossy().into_owned())) .collect(); let redacted_args = redact_sensitive_args(&args); let arg_strings: Vec<&str> = redacted_args.iter().map(CommandArg::as_str).collect(); info!("Running command: {} {}", program, arg_strings.join(" ")); let mut child = cmd.spawn()?; let stdout = child.stdout.take().expect("child stdout"); let stderr = child.stderr.take().expect("child stderr"); let out_handle = thread::spawn(move || { let reader = BufReader::new(stdout); let mut handle = io::stdout(); for line in reader.lines().map_while(Result::ok) { let _ = writeln!(handle, "{line}"); } }); let err_handle = thread::spawn(move || { let reader = BufReader::new(stderr); let mut handle = io::stderr(); for line in reader.lines().map_while(Result::ok) { let _ = writeln!(handle, "{line}"); } }); let status = child.wait()?; let _ = out_handle.join(); let _ = err_handle.join(); if status.success() { Ok(()) } else { #[expect( clippy::io_other_error, reason = "use explicit error kind for compatibility with older Rust" )] Err(io::Error::new( io::ErrorKind::Other, format!("ninja exited with {status}"), )) } }

Relevant snippet from tests/steps/cli_steps.rs (lines 104-107)

fn manifest_path(world: &mut CliWorld, path: String) { let cli = world.cli.as_ref().expect("cli"); assert_eq!(cli.file, PathBuf::from(&path)); }

codescene-delta-analysis · 2025-08-05T23:46:12Z

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: fake_ninja,fake_ninja_pwd

_Suppress

@coderabbitai Please create a new GitHub issue for this

🧩 Analysis chain

I'll create a GitHub issue to track the code duplication identified by the CodeScene analysis.

@leynos I've created a GitHub issue to track the code duplication identified by CodeScene. The issue documents the duplication between fake_ninja and fake_ninja_pwd functions and suggests extracting common functionality into a shared helper.

🏁 Script executed:

#!/bin/bash gh issue create \ --title "Refactor code duplication in test support utilities" \ --body "## Problem CodeScene analysis has identified code duplication between two functions in \`tests/support/mod.rs\`: - \`fake_ninja\` - \`fake_ninja_pwd\` Both functions follow a similar pattern for creating fake executables with temporary directories and shell scripts, but with different script content. ## Suggested Solution Consider extracting the common functionality into a shared helper function that accepts the script content as a parameter, reducing duplication whilst maintaining the specific behaviour of each function. ## Context This issue was identified during PR review and flagged by automated analysis. **References:** - PR: https://github.com/leynos/netsuke/pull/56 - Comment: https://github.com/leynos/netsuke/pull/56#discussion_r2255550299 **Requested by:** @leynos" \ --assignee leynos

Length of output: 935

@leynos Perfect! I've successfully created GitHub issue #63 to track the code duplication in the test support utilities. The issue documents the duplication between fake_ninja and fake_ninja_pwd functions and includes all the relevant context and references.

Issue URL: #63

The issue has been assigned to you and includes a suggested approach for refactoring the common functionality into a shared helper function.

🧬 Code Graph Analysis Results

Possibly relevant snippets from tests/steps/process_steps.rs (lines 32-35)

fn fake_ninja(world: &mut CliWorld, code: i32) { let (dir, path) = support::fake_ninja(code); install_test_ninja(world, dir, path); }

This function calls support::fake_ninja (from tests/support/mod.rs) to create a fake Ninja executable with a specified exit code.

It then installs this fake Ninja executable into the test environment represented by world.

Parameters:

world: mutable reference to a CliWorld test context.

code: integer exit code for the fake Ninja executable.

No return value.

Summary of tests/support/mod.rs (the provided file)

Provides test utilities for process management, especially for creating fake Ninja executables with various behaviors.

Functions include:

fake_ninja(exit_code: i32) -> (TempDir, PathBuf): creates a fake Ninja executable that exits with the given code.

fake_ninja_check_build_file() -> (TempDir, PathBuf): creates a fake Ninja that checks for the existence of a build file.

fake_ninja_pwd() -> (TempDir, PathBuf): creates a fake Ninja that writes its current directory to a file.

write_manifest(file: &mut impl Write) -> io::Result<()>: writes a minimal manifest to a writable stream.

Also includes a capture_logs utility to capture logs emitted during a closure execution.

Uses tempfile::TempDir for temporary directories and sets executable permissions on Unix.

Possibly relevant snippets from src/runner.rs (lines 23-25, 40-42, 53-55)

pub fn new(content: String) -> Self { Self(content) } pub fn new(arg: String) -> Self { Self(arg) } pub fn new(targets: Vec<String>) -> Self { Self(targets) }

These are simple constructor functions for some types wrapping strings or vectors of strings.

They may be relevant if the test utilities or fake executables interact with these types, but no direct connection is shown here.

No other snippets appear directly related to the tests/support/mod.rs file or its utilities.

-Original file line number
+Diff line change
@@ Expand Up / @@ -5,7 +5,7 @@ use cucumber::{given, then, when}; @@
     use netsuke::runner::{self, BuildTargets};
     use std::fs;
     use std::path::{Path, PathBuf};
-    use tempfile::TempDir;
+    use tempfile::{NamedTempFile, TempDir};
     /// Installs a test-specific ninja binary and updates the `PATH`.
     #[expect(
@@ Expand Down Expand Up / @@ -79,6 +79,29 @@ fn build_dir_exists(world: &mut CliWorld) { @@
     )]
     #[when("the ninja process is run")]
     fn run(world: &mut CliWorld) {
+        let dir = world.temp.as_ref().expect("temp dir");
+        let manifest_path = {
+            let cli = world.cli.as_ref().expect("cli");
+            if cli.file.is_absolute() {
+                cli.file.clone()
+            } else {
+                dir.path().join(&cli.file)
+            }
+        };
+        if !manifest_path.exists() {
+            let mut file =
+                NamedTempFile::new_in(dir.path()).expect("Failed to create temporary manifest file");
+            support::write_manifest(&mut file).expect("Failed to write manifest content");
+            file.persist(&manifest_path)
+                .expect("Failed to persist manifest file");
+        }
+        {
+            let cli = world.cli.as_mut().expect("cli");
+            cli.file.clone_from(&manifest_path);
+        }
         let cli = world.cli.as_ref().expect("cli");
         let program = if let Some(ninja) = &world.ninja {
             Path::new(ninja)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,4 +1,7 @@
-    //! Test utilities for process management and log capture.
+    //! Test utilities for process management.
+    //!
+    //! This module provides helpers for creating fake executables and
+    //! generating minimal manifests used in behavioural tests.
     use std::fs::{self, File};
     use std::io::{self, Write};
@@ Expand Down Expand Up / @@ -134,3 +137,23 @@ pub fn fake_ninja_pwd() -> (TempDir, PathBuf) { @@
         }
         (dir, path)
     }
+    /// Write a minimal manifest to `file`.
+    ///
+    /// The manifest declares a single `hello` target that prints a greeting.
+    /// This must be `allow` as `expect` will trigger an unfulfilled warning
+    /// despite the lint violation arising.
+    #[allow(dead_code, reason = "shared test utility not used in all crates")]
+    pub fn write_manifest(file: &mut impl Write) -> io::Result<()> {
+        writeln!(
+            file,
+            concat!(
+                "netsuke_version: \"1.0.0\"\n",
+                "targets:\n",
+                "  - name: hello\n",
+                "    recipe:\n",
+                "      kind: command\n",
+                "      command: \"echo hi\"\n"
+            ),
+        )
+    }

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle manifest path in process tests #56

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

coderabbitai Bot Aug 5, 2025 •

edited

Loading

Uh oh!

leynos Aug 5, 2025

Uh oh!

coderabbitai Bot Aug 5, 2025

Uh oh!

codescene-delta-analysis Bot Aug 5, 2025

Uh oh!

leynos Aug 5, 2025

Uh oh!

coderabbitai Bot Aug 5, 2025

Uh oh!

Uh oh!

Uh oh!

Handle manifest path in process tests #56

Uh oh!

Handle manifest path in process tests #56

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

coderabbitai Bot Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leynos Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 5, 2025

Choose a reason for hiding this comment

Relevant snippets from tests/support/mod.rs (lines 17-30, 41-65, 147-159)

Relevant snippet from src/runner.rs (lines 236-309)

Relevant snippet from tests/steps/cli_steps.rs (lines 104-107)

Uh oh!

codescene-delta-analysis Bot Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

leynos Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 5, 2025

Choose a reason for hiding this comment

Possibly relevant snippets from tests/steps/process_steps.rs (lines 32-35)

Summary of tests/support/mod.rs (the provided file)

Possibly relevant snippets from src/runner.rs (lines 23-25, 40-42, 53-55)

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Aug 5, 2025 •

edited

Loading

Relevant snippets from `tests/support/mod.rs` (lines 17-30, 41-65, 147-159)

Relevant snippet from `src/runner.rs` (lines 236-309)

Relevant snippet from `tests/steps/cli_steps.rs` (lines 104-107)

Possibly relevant snippets from `tests/steps/process_steps.rs` (lines 32-35)

Summary of `tests/support/mod.rs` (the provided file)

Possibly relevant snippets from `src/runner.rs` (lines 23-25, 40-42, 53-55)