Prevent manifest fixture overwrites (#61) - #545
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
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 (5 inconclusive)
✅ Passed checks (15 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📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
adfd1f1 to
d9f5dce
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
|
@coderabbitai review |
|
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.
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.
|
@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 |
|
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 outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Have the following now been resolved? test_support/src/manifest.rs (1)
♻️ Proposed extractionfn 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". 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current PR head =='
gh pr view 545 --repo leynos/netsuke --json headRefOid,updatedAt,url \
--jq '{headRefOid, updatedAt, url}'
echo '== Manifest symbol outline =='
ast-grep outline test_support/src/manifest.rs --items all --view expanded
echo '== Persistence and raced-target classification =='
rg -n -C 20 \
'fn\s+(persist_manifest_file|accept_raced_manifest_target|inspect_manifest_target|manifest_path_is_directory_error)\b|persist_noclobber' \
test_support/src/manifest.rs
echo '== Current PR diff for manifest implementation =='
gh pr diff 545 --repo leynos/netsuke --patch -- test_support/src/manifest.rsLength of output: 6527
Treat this finding as resolved on the current PR head,
This preserves the required behaviour. It removes the nested three-arm match from Do not make a further change for this finding. The source inspection confirms the extraction. The inspection sandbox did not run formatting, linting, or tests. You are interacting with an AI system. |
Persist staged manifest fixtures without replacing an existing target. Treat a concurrent creation as success and cover that no-clobber path.
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.
Describe preservation of existing and concurrently created file targets, directory rejection, atomic staging, and the controlled proof boundary.
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.
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.
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.
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.
Centralize raced-target classification and property-test error conversion without changing manifest creation or error-propagation behaviour.
6012d57 to
aeacfc6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
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: