Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6455b31fc9
ℹ️ 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".
| "rm --purge --dry-run must not create a delete marker" | ||
| ); | ||
|
|
||
| cleanup_bucket(config_dir.path(), &bucket_name); |
There was a problem hiding this comment.
Purge remaining version before cleanup
After asserting --dry-run, this test leaves keep-version.txt in a versioned bucket and then calls cleanup_bucket, which only runs rm --recursive --force and rb (no --purge). In a versioned bucket, that delete path does not remove historical versions, so bucket removal can fail and the test leaks buckets/versions on every successful run; over repeated CI or local runs this can exhaust test resources and cause unrelated integration failures. The same pattern is repeated in the object remove --purge --dry-run test.
Useful? React with 👍 / 👎.
Summary
This PR adds coverage for a recent
rm --purgepath that was still untested: running the command with--dry-runagainst a versioned object. Without this test, the purge feature only had assertions for real deletes, not for the safety path that should avoid both permanent deletion and delete-marker creation.The branch also adds a minimal root
Makefilewith apre-committarget because this repository did not exposemake pre-commit, while the documented local validation steps already existed as standalone cargo commands. The new target simply wraps the existing required checks.Root Cause
The recent purge work added permanent-delete behavior, but the dry-run interaction lived on a separate execution path and did not have focused regression coverage. That left a gap where a future change could accidentally route dry-run purge requests into the destructive path without a targeted test failing.
Fix
The new integration test enables versioning, uploads a single object, runs
rc rm --purge --dry-run --json, and then verifies that the object still has exactly one non-delete-marker version. This keeps the scope on the changedrm purgearea and exercises the user-visible behavior directly.Validation
cargo test --features integration --test integration option_behavior_operations::test_rm_purge_dry_run_does_not_delete_versioned_object -- --exact --nocapturemake pre-commit