fix(tui): set spillover root in dedup tests for Nix sandbox builds#1844
fix(tui): set spillover root in dedup tests for Nix sandbox builds#1844barstoolbluz wants to merge 1 commit into
Conversation
Two tool-result deduplication tests fail under `nix build` because the sandbox has no writable $HOME. The spillover write silently fails, dedup is skipped, and assertions break. Add a temp-dir-backed SpilloverRootGuard (matching the pattern in tool_result_retrieval.rs) so the tests pass in sandboxed and normal environments.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a SpilloverRootGuard RAII struct and updates unit tests in crates/tui/src/client/chat.rs to use it for managing temporary spillover directories safely with a mutex. The review feedback recommends adding the #[must_use] attribute to the guard to prevent accidental immediate drops and suggests using the SPILLOVER_DIR_NAME constant instead of hardcoded strings to improve maintainability.
| use crate::models::{ContentBlockStart, Delta, StreamEvent}; | ||
| use std::path::PathBuf; | ||
|
|
||
| struct SpilloverRootGuard { |
There was a problem hiding this comment.
RAII guards like SpilloverRootGuard should be marked with #[must_use] to prevent them from being accidentally dropped immediately (e.g., if the caller uses let _ = ... or forgets the binding), which would restore the prior state before the test logic executes.
| struct SpilloverRootGuard { | |
| #[must_use] | |
| struct SpilloverRootGuard { |
| .unwrap_or_else(|err| err.into_inner()); | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let _guard = SpilloverRootGuard::new( | ||
| tmp.path().join(".deepseek").join("tool_outputs"), |
There was a problem hiding this comment.
Use the SPILLOVER_DIR_NAME constant from crate::tools::truncate instead of hardcoding the string "tool_outputs". This ensures consistency if the directory name is changed in the future.
| tmp.path().join(".deepseek").join("tool_outputs"), | |
| tmp.path().join(".deepseek").join(crate::tools::truncate::SPILLOVER_DIR_NAME), |
| .unwrap_or_else(|err| err.into_inner()); | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let _guard = SpilloverRootGuard::new( | ||
| tmp.path().join(".deepseek").join("tool_outputs"), |
There was a problem hiding this comment.
Use the SPILLOVER_DIR_NAME constant from crate::tools::truncate instead of hardcoding the string "tool_outputs". This ensures consistency if the directory name is changed in the future.
| tmp.path().join(".deepseek").join("tool_outputs"), | |
| tmp.path().join(".deepseek").join(crate::tools::truncate::SPILLOVER_DIR_NAME), |
|
Independent review: The actual change is the right pattern — a temp-dir-backed Concerns:
The fix itself is correct and the pattern is reusable for any future test that exercises v0.8.48 (PR #2256): 1 conflict in |
3 participants
Summary
request_builder_deduplicates_large_identical_tool_results_with_retrieval_hintandcache_inspect_reports_tool_result_budget_metadata) fail undernix buildbecause the sandbox has no writable$HOMESpilloverRootGuard(matching the existing pattern intool_result_retrieval.rs) so the tests pass in both sandboxed and normal environmentsTest plan
cargo test -p deepseek-tui --bin deepseek-tui -- stream_decoder_tests::request_builder_deduplicates_large stream_decoder_tests::cache_inspect_reports_tool_resultpassesnix build .#succeeds (tests pass in sandbox)