Prevent manifest fixture overwrites (#61) - #545
Open
lodyai[bot] wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughManifest creation now classifies filesystem paths, preserves existing files, rejects directories, and propagates other errors. Tests cover missing targets, parent validation, persistence races, and non-overwriting behaviour. Documentation and Whitaker lint configuration were updated. ChangesManifest persistence
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 inconclusive)
✅ Passed checks (14 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Reviewer's GuideThis PR changes manifest fixture persistence to use a no-clobber atomic persist operation and adds a regression test to ensure existing manifest files are never overwritten while preserving race-tolerant behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Persist staged manifest fixtures without replacing an existing target. Treat a concurrent creation as success and cover that no-clobber path.
lodyai
Bot
force-pushed
the
issue-61-replace-namedtempfile-persist-with-more-explicit-file-creation-method
branch
from
August 9, 2026 02:43
adfd1f1 to
d9f5dce
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
August 9, 2026 02:53
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9f5dce7bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_support/src/manifest.rs`:
- Around line 269-274: Extract the repeated TempDir creation and UTF-8 path
conversion from the manifest tests into a shared rstest fixture returning the
temporary directory and manifest path. Apply the fixture to all four manifest
tests, including
persisting_manifest_tolerates_existing_file_without_overwriting, and replace
each duplicated setup with the fixture values.
- Around line 107-109: Update the AlreadyExists branch in persist_manifest_file
to inspect manifest_path: return an IsADirectory error when the raced target is
a directory, and preserve success only for an existing non-directory target. Add
a regression test covering persist_manifest_file with a directory destination.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bda5f391-8665-4f48-b782-531f57942afa
📒 Files selected for processing (1)
test_support/src/manifest.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Verify targets after no-clobber persistence reports `AlreadyExists` so existing files remain acceptable while directory targets are rejected. Share manifest test workspace setup and cover the raced-directory path.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope). ❌ Failed checks (3 warnings, 4 inconclusive)
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_support/src/manifest.rs`:
- Around line 102-105: Update the public `ensure_manifest_exists` Rustdoc to
explicitly document that it does not overwrite an existing target and tolerates
a concurrently created non-directory target as satisfying the manifest-exists
contract. Retain the private helper’s documentation for implementation details.
- Around line 289-334: Extend the public-boundary tests for
ensure_manifest_exists rather than testing persist_manifest_file directly. Add a
test-only seam immediately before persist_manifest_file that deterministically
creates the target file or directory, then assert existing file contents remain
unchanged and directory targets return IsADirectory with the target path in the
error. Cover all five bounded race states as explicit deterministic cases,
avoiding scheduling-dependent tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99b8e563-24d3-4ed4-b84c-857f156fb547
📒 Files selected for processing (1)
test_support/src/manifest.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
Describe preservation of existing and concurrently created file targets, directory rejection, atomic staging, and the controlled proof boundary.
Collaborator
|
@coderabbitai review |
Contributor
|
Document the no-clobber contract and exercise all controlled target creation orderings through a test-only persistence seam. Keep generated staged content, existing files, and directory errors under bounded property coverage without asserting arbitrary scheduler behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Inspect manifest paths through a fallible test-support filesystem wrapper so inaccessible metadata is never mistaken for an absent target. Exercise the exported manifest helper with a scoped pre-persist hook and document the controlled no-clobber contract for fixture authors.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on lines +90 to +103 fn inspect_path_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyhow::Result<()> {
let temp = temp_dir?;
let file = temp.path().join("regular-file");
write(&file, b"fixture")?;
let Err(error) = inspect_path(file.join("child")) else {
anyhow::bail!("traversing through a regular file should fail");
};
anyhow::ensure!(
error.kind() != io::ErrorKind::NotFound,
"traversal through a file should not be reported as absence, got {error:?}"
);
Ok(())
}❌ New issue: Code Duplication |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and changed files =='
gh pr view 545 --repo leynos/netsuke \
--json number,url,headRefName,baseRefName,updatedAt,files,statusCheckRollup \
--jq '{
number, url, headRefName, baseRefName, updatedAt,
files: [.files[] | {path, additions, deletions}],
checks: [.statusCheckRollup[]? | {name, status, conclusion, detailsUrl}]
}'
echo '== Relevant files =='
fd -t f -a '^(manifest\.rs|tests\.rs|fs\.rs|lib\.rs|developers-guide\.md|Cargo\.toml|AGENTS\.md)$' \
test_support docs . 2>/dev/null | sort
echo '== Manifest module outline =='
ast-grep outline test_support/src/manifest.rs --items all --view expanded 2>/dev/null || true
echo '== Manifest implementation and tests =='
cat -n test_support/src/manifest.rs 2>/dev/null || true
printf '\n== Separate manifest tests, if present ==\n'
cat -n test_support/src/manifest/tests.rs 2>/dev/null || true
echo '== Filesystem wrapper API =='
ast-grep outline test_support/src/fs.rs --items all --view expanded 2>/dev/null || true
rg -n -C 5 \
'pub\s+(?:enum|fn)\s+(?:Target|.*metadata|.*exist|.*dir)|\b(?:metadata|symlink_metadata|exists|is_dir)\s*\(' \
test_support/src/fs.rs test_support/src/manifest.rs 2>/dev/null || true
echo '== Public API exports and documentation =='
rg -n -C 5 \
'\bensure_manifest_exists\b|no-clobber|noclobber|concurrent|raced|IsADirectory|manifest fixture' \
test_support/src/lib.rs test_support/src/manifest.rs docs/developers-guide.md 2>/dev/null || true
echo '== Property-test dependency and manifest test usage =='
rg -n -C 4 \
'^(proptest|rstest)\s*=|\bproptest!\s*\{|TestCaseError|ensure_manifest_exists(?:\s*\()?' \
Cargo.toml test_support/Cargo.toml test_support/src/manifest.rs test_support/src/manifest/tests.rs 2>/dev/null || true
echo '== Linked issue =='
gh issue view 61 --repo leynos/netsuke --json number,state,title,url \
--jq '{number,state,title,url}'Length of output: 70971
Do not create a new GitHub issue. Open issue GitHub CI shows a successful ✏️ Learnings added
You are interacting with an AI system. |
This comment was marked as resolved.
This comment was marked as resolved.
Share the file-traversal assertion while keeping the named tests for the `inspect_path` and `try_is_file` public contracts. Forward Whitaker's root configuration to its isolated driver so its documented scoped exclusions remain effective.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_support/src/fs.rs`:
- Around line 131-164: Update the Rustdoc for inspect_path to explicitly state
that fs::metadata follows symlinks, so dangling symlinks are reported as
PathState::Absent even when the directory entry exists. Keep the implementation
unchanged and document this contract near the behavior description or examples.
In `@test_support/src/manifest.rs`:
- Around line 80-91: Update the Rustdoc for install_before_persist_hook to
explicitly state that the installed action is one-shot and is removed after
run_before_persist_hook executes, so it runs at most once per guard even across
multiple ensure_manifest_exists calls.
- Around line 183-190: Update both PathState::NonDirectory arms in
ensure_parent_directory (test_support/src/manifest.rs:183-190) and
find_existing_ancestor (test_support/src/manifest.rs:234-241) to return
io::ErrorKind::NotADirectory instead of AlreadyExists, preserving their existing
error messages and control flow.
In `@test_support/src/manifest/tests.rs`:
- Around line 120-170: Extend both raced persistence tests,
raced_file_manifest_path_is_returned_unchanged and
raced_directory_manifest_path_is_rejected, to assert that the temporary
workspace directory contains no leftover staged temporary file after
ensure_manifest_exists returns or errors. Use the existing temp_path workspace
root and directory-inspection utilities, while preserving the current content,
error-kind, and returned-path assertions.
- Around line 343-347: Update the TargetState::ExistingDirectory |
TargetState::RacedDirectory branch to validate result is an error with
prop_assert! before extracting it, replacing result.expect_err. Preserve the
existing IsADirectory and expected_path assertions after safely obtaining the
error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfc60452-dd35-4e7b-a09a-79a63e96e3a2
📒 Files selected for processing (6)
Makefiledocs/developers-guide.mdtest_support/src/fs.rstest_support/src/fs_tests.rstest_support/src/manifest.rstest_support/src/manifest/tests.rs
Report non-directory parent components as `NotADirectory` and cover both internal branches. Confirm raced persistence removes staged files, improve the property assertion, and document the filesystem and hook contracts.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_support/src/manifest.rs (1)
151-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the raced-target classification into a named helper.
persist_manifest_filenow nests a three-armPathStatematch inside a three-arm persistence match. The function mixes two responsibilities: persistence and post-race target classification. Extract theAlreadyExistshandling into a helper so each function keeps one responsibility and the nesting stays shallow.♻️ Proposed extraction
fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::Result<()> { match file.persist_noclobber(manifest_path.as_std_path()) { Ok(_) => Ok(()), - Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => { - match inspect_manifest_target(manifest_path)? { - fs::PathState::Directory => Err(manifest_path_is_directory_error(manifest_path)), - fs::PathState::NonDirectory => Ok(()), - fs::PathState::Absent => Err(io::Error::new( - io::ErrorKind::AlreadyExists, - format!( - "Manifest target disappeared after no-clobber persistence reported it existed: {manifest_path}" - ), - )), - } - } + Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => { + accept_raced_manifest_target(manifest_path) + } Err(e) => Err(io::Error::new( e.error.kind(), format!( "Failed to persist manifest file to {} from {}: {}", manifest_path, e.file.path().display(), e.error ), )), } } + +/// Classify a target that no-clobber persistence reported as already present. +fn accept_raced_manifest_target(manifest_path: &Utf8Path) -> io::Result<()> { + match inspect_manifest_target(manifest_path)? { + fs::PathState::Directory => Err(manifest_path_is_directory_error(manifest_path)), + fs::PathState::NonDirectory => Ok(()), + fs::PathState::Absent => Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "Manifest target disappeared after no-clobber persistence reported it existed: {manifest_path}" + ), + )), + } +}As per path instructions, "Seek to keep the cognitive complexity of functions no more than 9", "Adhere to single responsibility and CQRS", and "Move conditionals with >2 branches into a predicate function".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test_support/src/manifest.rs` around lines 151 - 176, Extract the AlreadyExists branch from persist_manifest_file into a named helper that accepts manifest_path, performs the inspect_manifest_target classification, and returns the corresponding io::Result. Replace the nested match arm with a call to this helper, preserving the Directory, NonDirectory, and Absent behaviors and leaving other persistence errors unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_support/src/manifest/tests.rs`:
- Around line 342-396: Extract a shared conversion helper near the property test
that turns an error into TestCaseError::fail(error.to_string()), then replace
the repeated inline map_err closures throughout property_workspace and each
result branch with that helper. Preserve the existing error propagation and
assertion behavior while making each conversion a single helper call.
---
Outside diff comments:
In `@test_support/src/manifest.rs`:
- Around line 151-176: Extract the AlreadyExists branch from
persist_manifest_file into a named helper that accepts manifest_path, performs
the inspect_manifest_target classification, and returns the corresponding
io::Result. Replace the nested match arm with a call to this helper, preserving
the Directory, NonDirectory, and Absent behaviors and leaving other persistence
errors unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f732dd7-edf4-49b4-a75c-5aad3a037484
📒 Files selected for processing (6)
Makefiledocs/developers-guide.mdtest_support/src/fs.rstest_support/src/fs_tests.rstest_support/src/manifest.rstest_support/src/manifest/tests.rs
Comment on lines
+342
to
+396
| let (_temp, temp_path) = property_workspace()?; | ||
| let cli_file = Utf8Path::new("manifest.yml"); | ||
| let expected_path = temp_path.join(cli_file); | ||
| let staged_contents = manifest_yaml(&format!( | ||
| "targets:\n - name: {staged_name}\n command: \"echo hi\"\n" | ||
| )) | ||
| .into_bytes(); | ||
|
|
||
| match state { | ||
| TargetState::ExistingFile => fs::write(expected_path.as_std_path(), &competing_contents) | ||
| .map_err(|error| TestCaseError::fail(error.to_string()))?, | ||
| TargetState::ExistingDirectory => fs::create_dir(expected_path.as_std_path()) | ||
| .map_err(|error| TestCaseError::fail(error.to_string()))?, | ||
| TargetState::Missing | TargetState::RacedFile | TargetState::RacedDirectory => {} | ||
| } | ||
|
|
||
| let expected_staged_contents = staged_contents.clone(); | ||
| let hook_contents = competing_contents.clone(); | ||
| let _hook = install_before_persist_hook(move |file, manifest_path| { | ||
| let replacement = replace_staged_manifest(file, manifest_path, &staged_contents)?; | ||
| match state { | ||
| TargetState::RacedFile => { | ||
| fs::write(manifest_path.as_std_path(), hook_contents)?; | ||
| } | ||
| TargetState::RacedDirectory => fs::create_dir(manifest_path.as_std_path())?, | ||
| TargetState::Missing | ||
| | TargetState::ExistingFile | ||
| | TargetState::ExistingDirectory => {} | ||
| } | ||
| Ok(replacement) | ||
| }); | ||
| let result = ensure_manifest_exists(&temp_path, cli_file); | ||
|
|
||
| match state { | ||
| TargetState::Missing => { | ||
| let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; | ||
| prop_assert_eq!(&returned_path, &expected_path); | ||
| let contents = fs::read(expected_path.as_std_path()) | ||
| .map_err(|error| TestCaseError::fail(error.to_string()))?; | ||
| prop_assert_eq!(contents, expected_staged_contents); | ||
| } | ||
| TargetState::ExistingFile | TargetState::RacedFile => { | ||
| let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; | ||
| prop_assert_eq!(&returned_path, &expected_path); | ||
| let contents = fs::read(expected_path.as_std_path()) | ||
| .map_err(|error| TestCaseError::fail(error.to_string()))?; | ||
| prop_assert_eq!(contents, competing_contents); | ||
| } | ||
| TargetState::ExistingDirectory | TargetState::RacedDirectory => { | ||
| prop_assert!(result.is_err(), "directory target should be rejected"); | ||
| let error = result | ||
| .err() | ||
| .ok_or_else(|| TestCaseError::fail("directory target should be rejected"))?; | ||
| prop_assert_eq!(error.kind(), io::ErrorKind::IsADirectory); | ||
| prop_assert!(error.to_string().contains(expected_path.as_str())); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the repeated TestCaseError::fail(error.to_string()) mapping with one helper.
The property body repeats .map_err(|error| TestCaseError::fail(error.to_string())) eight times. Extract a single conversion helper and reuse it. The property assertions then read as one statement each.
♻️ Proposed helper
+fn fail(error: impl std::fmt::Display) -> TestCaseError {
+ TestCaseError::fail(error.to_string())
+}- let (_temp, temp_path) = property_workspace()?;
+ let (_temp, temp_path) = property_workspace()?;
@@
- TargetState::ExistingFile => fs::write(expected_path.as_std_path(), &competing_contents)
- .map_err(|error| TestCaseError::fail(error.to_string()))?,
- TargetState::ExistingDirectory => fs::create_dir(expected_path.as_std_path())
- .map_err(|error| TestCaseError::fail(error.to_string()))?,
+ TargetState::ExistingFile =>
+ fs::write(expected_path.as_std_path(), &competing_contents).map_err(fail)?,
+ TargetState::ExistingDirectory =>
+ fs::create_dir(expected_path.as_std_path()).map_err(fail)?,Apply the same substitution in property_workspace and in each result branch.
As per path instructions, tests must avoid repetition; the guideline requires rstest fixtures and shared helpers "to avoid repetition between tests".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (_temp, temp_path) = property_workspace()?; | |
| let cli_file = Utf8Path::new("manifest.yml"); | |
| let expected_path = temp_path.join(cli_file); | |
| let staged_contents = manifest_yaml(&format!( | |
| "targets:\n - name: {staged_name}\n command: \"echo hi\"\n" | |
| )) | |
| .into_bytes(); | |
| match state { | |
| TargetState::ExistingFile => fs::write(expected_path.as_std_path(), &competing_contents) | |
| .map_err(|error| TestCaseError::fail(error.to_string()))?, | |
| TargetState::ExistingDirectory => fs::create_dir(expected_path.as_std_path()) | |
| .map_err(|error| TestCaseError::fail(error.to_string()))?, | |
| TargetState::Missing | TargetState::RacedFile | TargetState::RacedDirectory => {} | |
| } | |
| let expected_staged_contents = staged_contents.clone(); | |
| let hook_contents = competing_contents.clone(); | |
| let _hook = install_before_persist_hook(move |file, manifest_path| { | |
| let replacement = replace_staged_manifest(file, manifest_path, &staged_contents)?; | |
| match state { | |
| TargetState::RacedFile => { | |
| fs::write(manifest_path.as_std_path(), hook_contents)?; | |
| } | |
| TargetState::RacedDirectory => fs::create_dir(manifest_path.as_std_path())?, | |
| TargetState::Missing | |
| | TargetState::ExistingFile | |
| | TargetState::ExistingDirectory => {} | |
| } | |
| Ok(replacement) | |
| }); | |
| let result = ensure_manifest_exists(&temp_path, cli_file); | |
| match state { | |
| TargetState::Missing => { | |
| let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; | |
| prop_assert_eq!(&returned_path, &expected_path); | |
| let contents = fs::read(expected_path.as_std_path()) | |
| .map_err(|error| TestCaseError::fail(error.to_string()))?; | |
| prop_assert_eq!(contents, expected_staged_contents); | |
| } | |
| TargetState::ExistingFile | TargetState::RacedFile => { | |
| let returned_path = result.map_err(|error| TestCaseError::fail(error.to_string()))?; | |
| prop_assert_eq!(&returned_path, &expected_path); | |
| let contents = fs::read(expected_path.as_std_path()) | |
| .map_err(|error| TestCaseError::fail(error.to_string()))?; | |
| prop_assert_eq!(contents, competing_contents); | |
| } | |
| TargetState::ExistingDirectory | TargetState::RacedDirectory => { | |
| prop_assert!(result.is_err(), "directory target should be rejected"); | |
| let error = result | |
| .err() | |
| .ok_or_else(|| TestCaseError::fail("directory target should be rejected"))?; | |
| prop_assert_eq!(error.kind(), io::ErrorKind::IsADirectory); | |
| prop_assert!(error.to_string().contains(expected_path.as_str())); | |
| let (_temp, temp_path) = property_workspace()?; | |
| let cli_file = Utf8Path::new("manifest.yml"); | |
| let expected_path = temp_path.join(cli_file); | |
| let staged_contents = manifest_yaml(&format!( | |
| "targets:\n - name: {staged_name}\n command: \"echo hi\"\n" | |
| )) | |
| .into_bytes(); | |
| match state { | |
| TargetState::ExistingFile => | |
| fs::write(expected_path.as_std_path(), &competing_contents).map_err(fail)?, | |
| TargetState::ExistingDirectory => | |
| fs::create_dir(expected_path.as_std_path()).map_err(fail)?, | |
| TargetState::Missing | TargetState::RacedFile | TargetState::RacedDirectory => {} | |
| } | |
| let expected_staged_contents = staged_contents.clone(); | |
| let hook_contents = competing_contents.clone(); | |
| let _hook = install_before_persist_hook(move |file, manifest_path| { | |
| let replacement = replace_staged_manifest(file, manifest_path, &staged_contents)?; | |
| match state { | |
| TargetState::RacedFile => { | |
| fs::write(manifest_path.as_std_path(), hook_contents)?; | |
| } | |
| TargetState::RacedDirectory => fs::create_dir(manifest_path.as_std_path())?, | |
| TargetState::Missing | |
| | TargetState::ExistingFile | |
| | TargetState::ExistingDirectory => {} | |
| } | |
| Ok(replacement) | |
| }); | |
| let result = ensure_manifest_exists(&temp_path, cli_file); | |
| match state { | |
| TargetState::Missing => { | |
| let returned_path = result.map_err(fail)?; | |
| prop_assert_eq!(&returned_path, &expected_path); | |
| let contents = fs::read(expected_path.as_std_path()).map_err(fail)?; | |
| prop_assert_eq!(contents, expected_staged_contents); | |
| } | |
| TargetState::ExistingFile | TargetState::RacedFile => { | |
| let returned_path = result.map_err(fail)?; | |
| prop_assert_eq!(&returned_path, &expected_path); | |
| let contents = fs::read(expected_path.as_std_path()).map_err(fail)?; | |
| prop_assert_eq!(contents, competing_contents); | |
| } | |
| TargetState::ExistingDirectory | TargetState::RacedDirectory => { | |
| prop_assert!(result.is_err(), "directory target should be rejected"); | |
| let error = result | |
| .err() | |
| .ok_or_else(|| TestCaseError::fail("directory target should be rejected"))?; | |
| prop_assert_eq!(error.kind(), io::ErrorKind::IsADirectory); | |
| prop_assert!(error.to_string().contains(expected_path.as_str())); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_support/src/manifest/tests.rs` around lines 342 - 396, Extract a shared
conversion helper near the property test that turns an error into
TestCaseError::fail(error.to_string()), then replace the repeated inline map_err
closures throughout property_workspace and each result branch with that helper.
Preserve the existing error propagation and assertion behavior while making each
conversion a single helper call.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch replaces overwrite-capable manifest persistence with an
explicit no-clobber operation. It retains atomic same-directory staging and
treats a target created concurrently as satisfying the manifest contract.
Closes #61.
Review walkthrough
Validation
cargo nextest run -p test_support --all-features: 74 passed.cargo nextest run --test bdd_tests --all-features: 245 passed.make check-fmt,make test,make typecheck, andmake lint: passed.coderabbit review --agent: 0 findings.References
Summary by Sourcery
Prevent manifest fixture persistence from overwriting existing manifest files while maintaining race-tolerant behavior.
Bug Fixes:
Tests: