Propagate silently-swallowed errors in credential/profile writes - #229
Merged
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughThe changes replace silently ignored profile, filesystem durability, metadata-writing, and isolate linking failures with explicit errors, cleanup, early returns, or warnings. ChangesError handling improvements
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@src/atomic_fs.rs`:
- Around line 96-120: Update the atomic write flow around the temporary-file
creation and cleanup so `remove_file(&tmp)` runs only after this invocation
successfully creates the temp file; handle `OpenOptions::open` separately before
the closure, preserving `AlreadyExists` without deleting another writer’s file.
When cleanup is required, propagate or combine any cleanup failure with the
original operation error instead of discarding it.
In `@src/auth.rs`:
- Around line 1083-1085: Update the Windows symlink_or_copy function to detect
src.is_dir() before invoking fs::copy. For directory sources, use the existing
directory-aware symlink or copy behavior so isolate directories such as .cache
and Documents are preserved; retain the current fs::copy path for files and pass
the resulting status through warn_on_link_failure.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 058c581c-d797-469d-83e7-4684d8c5f192
📒 Files selected for processing (3)
src/alias.rssrc/atomic_fs.rssrc/auth.rs
Comment on lines
+96
to
120
| let result = (|| -> std::io::Result<()> { | ||
| { | ||
| let mut f = std::fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .open(&tmp)?; | ||
| f.write_all(bytes)?; | ||
| f.flush()?; | ||
| f.sync_all()?; | ||
| } | ||
|
|
||
| if let Some(perms) = permissions { | ||
| let _ = std::fs::set_permissions(&tmp, perms.clone()); | ||
| } | ||
| if let Some(perms) = permissions { | ||
| std::fs::set_permissions(&tmp, perms.clone())?; | ||
| } | ||
|
|
||
| // std::fs::rename already replaces an existing destination on every | ||
| // platform we build for (including Windows, via MoveFileExW's | ||
| // MOVEFILE_REPLACE_EXISTING) — no separate pre-removal needed, and one | ||
| // would only open a window where neither the old nor new file exists. | ||
| if let Err(e) = std::fs::rename(&tmp, path) { | ||
| // Don't leave a half-written temp behind before the caller decides | ||
| // whether to fall back. | ||
| // std::fs::rename already replaces an existing destination on every | ||
| // platform we build for (including Windows, via MoveFileExW's | ||
| // MOVEFILE_REPLACE_EXISTING) — no separate pre-removal needed, and one | ||
| // would only open a window where neither the old nor new file exists. | ||
| std::fs::rename(&tmp, path) | ||
| })(); | ||
|
|
||
| if result.is_err() { | ||
| let _ = std::fs::remove_file(&tmp); | ||
| return Err(e); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only clean up a temp file after this invocation successfully creates it.
Because open(&tmp)? is inside the closure, AlreadyExists also reaches Line 119 and may delete another concurrent writer’s temp file. Move creation outside the closure, then surface any cleanup failure alongside the original error rather than discarding it.
🤖 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 `@src/atomic_fs.rs` around lines 96 - 120, Update the atomic write flow around
the temporary-file creation and cleanup so `remove_file(&tmp)` runs only after
this invocation successfully creates the temp file; handle `OpenOptions::open`
separately before the closure, preserving `AlreadyExists` without deleting
another writer’s file. When cleanup is required, propagate or combine any
cleanup failure with the original operation error instead of discarding it.
Comment on lines
1083
to
+1085
| #[cfg(windows)] | ||
| fn symlink_or_copy(src: &std::path::Path, dest: &std::path::Path) { | ||
| fs::copy(src, dest).ok(); | ||
| warn_on_link_failure(src, dest, fs::copy(src, dest).map(|_| ())); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
curl -fsSL https://doc.rust-lang.org/std/fs/fn.copy.html |
python -c 'import sys; text=sys.stdin.read(); assert "neither a regular file nor a symlink to a regular file" in text'Repository: getappz/agentflare
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant function and surrounding lines.
sed -n '1040,1125p' src/auth.rs
printf '\n---- CALLERS ----\n'
# Find call sites for symlink_or_copy.
grep -RIn --exclude-dir=.git "symlink_or_copy" src || trueRepository: getappz/agentflare
Length of output: 3518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the repository for `symlink_or_copy`.
grep -RIn --exclude-dir=.git "symlink_or_copy" .
printf '\n---- src/auth.rs around the symbol ----\n'
grep -n "symlink_or_copy" -n src/auth.rs || trueRepository: getappz/agentflare
Length of output: 797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '760,835p' src/auth.rsRepository: getappz/agentflare
Length of output: 2523
Handle directory sources on Windows before calling fs::copy
fs::copy skips directories, so the Windows isolate never links .cache, .config, .local, Documents, Downloads, or .ssh; those paths are just warned about and omitted. Use a directory-aware copy/symlink path when src.is_dir().
🤖 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 `@src/auth.rs` around lines 1083 - 1085, Update the Windows symlink_or_copy
function to detect src.is_dir() before invoking fs::copy. For directory sources,
use the existing directory-aware symlink or copy behavior so isolate directories
such as .cache and Documents are preserved; retain the current fs::copy path for
files and pass the resulting status through warn_on_link_failure.
getappz
enabled auto-merge (squash)
July 17, 2026 19:28
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three places where errors that matter were silently swallowed, defeating the durability/integrity guarantees the surrounding code advertises. All are in security-/data-sensitive paths (credential vault writes, the user's shell profile, isolate setup).
1.
atomic_fs.rs— durability & permission errors dropped. The module doc promises a "durable, crash-atomic write" for credential files, yet bothtry_atomic_writeandin_place_overwritedid:A dropped
sync_allmeans the "durable" write may never reach disk; a droppedset_permissionsmeans a credential file the doc explicitly wants at0600can silently be left world-readable. These now propagate via?.try_atomic_write's write/perms/rename sequence is wrapped in a closure so the temp file is still cleaned up on any failure (previously only the rename path cleaned up).2.
alias.rs— unreadable shell profile treated as empty (data loss).run()didstd::fs::read_to_string(&profile).ok(), collapsing a real read error intoNone.write_managed_blockthen treatsNoneas empty content and overwrites the file with just the managed block — so an existing but unreadable~/.bashrc(permissions, transient I/O error) would be clobbered. Now onlyNotFoundmaps toNone; any other error aborts with a message (respecting the existing--jsonoutput contract).3.
auth.rs— silent failures during isolate setup.symlink_or_copydid.ok(), so a failure to link.ssh/.gitconfig/.git-credentialsinto an isolate passed silently, leaving the isolate missing the host state it needs. Theisolate.jsonmetadata write also used.ok(), and without itread_isolate_modelater can't tell shallow from deep. Both now emit warnings (an already-present link target is still treated as fine).Test plan
cargo test(all pass)cargo clippy --locked --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic(clean; matches CI's gate)cargo fmt --checkNotes for reviewers
sync_all/set_permissionsnow turns previously-ignored failures into real errors. On normal filesystems these don't fail; on exotic ones (some network FS) async_allerror would now surface instead of a silent, non-durable write — which is the intended behavior for this module.write_bytes_with_fallbackalready wraps and reports these.alias.rsnow exits non-zero (or emits anerrorstatus in--jsonmode) instead of clobbering an unreadable profile; the isolate changes only add warnings on the previously-silent failure paths.Link to Devin session: https://app.devin.ai/sessions/e555a41244c34e41a11e2b4cfa4cf706
Requested by: @getappz
Summary by CodeRabbit