test(userdata): add regression tests for atomic write and invalid filename fixes - #15815
test(userdata): add regression tests for atomic write and invalid filename fixes#15815chelsealong wants to merge 1 commit into
Conversation
…ename fixes Covers two of the userdata bugs flagged in Comfy-Org#15814 as fixed without regression tests: non-atomic writes truncating the original file on failure (9a870b5), and invalid filenames surfacing a 500 instead of a 400 (0737b7e). The remaining items in that issue reference commit hashes that don't exist on this branch, so they're left out. Ref Comfy-Org#15814
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
🧰 Additional context used📓 Path-based instructions (5)**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{py,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{py,md,txt,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
🪛 ast-grep (0.45.1)tests-unit/prompt_server_test/user_manager_test.py[warning] 290-290: File path is request-/variable-derived; validate and normalize to prevent path traversal. (open-filename-from-request) [warning] 300-300: File path is request-/variable-derived; validate and normalize to prevent path traversal. (open-filename-from-request) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughAdded unit tests for user manager write failures. The tests verify that overlong filenames return HTTP 400 without creating a file. They also verify that failed atomic replacement returns HTTP 400, preserves the original file contents, and removes temporary files. Merge Risk: ⚪ Minimal · up to This change adds focused regression coverage without modifying production behavior, so no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Summary
#15814 audits recent bug-fix history and lists six merged fixes that shipped
without regression tests. This PR adds tests for the two items whose commits
I could verify still exist and are still relevant on
master:9a870b510) —POST /userdata/{file}writesto a temp file and
os.replace()s it into place so a mid-write failurecan't truncate the original file.
0737b7e0d) — anOSErrorwhile writing(e.g. a filename that's too long, or contains characters the filesystem
rejects) is now reported as
400, not an unhandled500.The other four items in the issue (
24dc581dc,136c93cb4,8e2c99e3c,89f15894d) either already have a passing regression handled byexist_ok=Truewith nothing meaningfully new to assert, or reference commit hashes that don't
exist in this repository's history — I couldn't find the code they describe,
so I left them out rather than guess.
Test plan
New tests added to
tests-unit/prompt_server_test/user_manager_test.py:test_post_userdata_invalid_filename_returns_400test_post_userdata_atomic_write_preserves_original_on_failureVerified each test fails without its corresponding fix, by checking out
app/user_manager.pyfrom the commit before each fix and re-running:And that both pass, along with the full existing file, on current
master:Lint:
AI disclosure
This PR was prepared with the assistance of an AI coding agent (Claude),
including the analysis of which issue items were still reproducible and the
test implementation. All test runs and diffs shown above were executed and
verified directly.