Prevent manifest fixture overwrites (#61) - #545
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 persistence now creates files without overwriting existing manifests. It tolerates concurrent creation, rejects directory targets, and preserves other persistence errors. Tests use a shared workspace fixture and cover these behaviours. ChangesManifest persistence
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 4 warnings, 2 inconclusive)
✅ Passed checks (12 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 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 |
![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.
Persist staged manifest fixtures without replacing an existing target. Treat a concurrent creation as success and cover that no-clobber path.
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.
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.
|
@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)
Describe preservation of existing and concurrently created file targets, directory rejection, atomic staging, and the controlled proof boundary.
|
@coderabbitai review |
|
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.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| fs_tests.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current PR and target file status =='
gh pr view 545 --repo leynos/netsuke --json number,headRefName,updatedAt,url \
--jq '{number, headRefName, updatedAt, url}'
git status --short
echo '== Filesystem test module outline =='
ast-grep outline test_support/src/fs_tests.rs --items all --view expanded
echo '== Reported tests and nearby helpers =='
rg -n -C 18 \
'fn\s+(inspect_path_propagates_errors_other_than_not_found|try_is_file_propagates_errors_other_than_not_found)\b|fn\s+.*propagates_errors_other_than_not_found\b' \
test_support/src/fs_tests.rs
echo '== Test-module imports and complete local helper map =='
sed -n '1,240p' test_support/src/fs_tests.rs
echo '== Filesystem API symbols under test =='
ast-grep outline test_support/src/fs.rs --items all --view expanded
rg -n -C 6 \
'pub\s+(?:enum|fn)\s+(?:PathState|inspect_path|try_is_file)\b|fn\s+(?:inspect_path|try_is_file)\b' \
test_support/src/fs.rs test_support/src/fs_tests.rsLength of output: 13210
Fix this duplication. The current PR branch contains identical setup and assertions in both tests. Keep both test names. Extract the shared setup and non- Use a generic closure so the helper accepts both return types: fn assert_traversal_through_file_propagates_non_not_found<T>(
temp_dir: TempDir,
inspect: impl FnOnce(&Path) -> io::Result<T>,
) -> anyhow::Result<()> {
let temp = temp_dir?;
let file = temp.path().join("regular-file");
write(&file, b"fixture")?;
let Err(error) = inspect(&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(())
}Import assert_traversal_through_file_propagates_non_not_found(temp_dir, try_is_file)assert_traversal_through_file_propagates_non_not_found(temp_dir, inspect_path)This change keeps separate contract coverage for Use this AI coding-agent prompt: This finding is in scope for PR 🧠 Learnings usedYou are interacting with an AI system. |
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: