Skip to content

Propagate silently-swallowed errors in credential/profile writes #229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
getappz merged 2 commits into from
Jul 17, 2026
Merged

Propagate silently-swallowed errors in credential/profile writes #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
990a8ad
Propagate silently-swallowed errors in credential/profile writes
getappz Jul 17, 2026
7f35f67
Merge branch 'master' into devin/1784314581-error-handling
getappz Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/alias.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,30 @@ pub fn run(
}
};

let existing_content = std::fs::read_to_string(&profile).ok();
// Distinguish "file doesn't exist yet" (fine — we'll create it) from a real
// read failure. Collapsing both to None via .ok() would treat an unreadable
// existing profile as empty and then overwrite the user's real shell profile
// with just the managed block, destroying its contents.
let existing_content = match std::fs::read_to_string(&profile) {
Ok(content) => Some(content),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => None,
Err(e) => {
let err = format!("cannot read profile {}: {e}", profile.display());
if !json {
eprintln!("error: {err}");
std::process::exit(1);
}
emit_json(JsonOutput {
requested: preferred.to_string(),
installed: String::new(),
status: Status::WriteError.as_str(),
profile: profile.to_string_lossy().into_owned(),
snippet: None,
error: Some(err),
});
return;
}
};

if !force
&& let Some(ref content) = existing_content
Expand Down
55 changes: 31 additions & 24 deletions src/atomic_fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,38 @@ pub fn try_atomic_write(
.map_or(0, |d| d.as_nanos());
let tmp = parent.join(format!(".{filename}.agentflare.tmp.{pid}.{nanos}"));

{
let mut f = std::fs::OpenOptions::new()
.write(true)
.create_new(true)
.open(&tmp)?;
f.write_all(bytes)?;
let _ = f.flush();
let _ = f.sync_all();
}
// Everything after the temp file is created must clean it up on failure so
// a half-written temp isn't left behind before the caller decides whether
// to fall back. Errors from flush/sync_all/set_permissions are propagated
// rather than swallowed: a silently-dropped sync_all defeats the whole
// durability guarantee, and a silently-dropped set_permissions would leave
// e.g. a credential file at the process default mode instead of 0600.
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);
}
Comment on lines +96 to 120

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Ok(())
result
}

/// In-place overwrite of an existing file inode (`O_WRONLY|O_TRUNC`, plus
Expand All @@ -137,11 +144,11 @@ pub fn in_place_overwrite(

let mut f = opts.open(path)?;
f.write_all(bytes)?;
let _ = f.flush();
let _ = f.sync_all();
f.flush()?;
f.sync_all()?;

if let Some(perms) = permissions {
let _ = std::fs::set_permissions(path, perms.clone());
std::fs::set_permissions(path, perms.clone())?;
}
Ok(())
}
Expand Down
32 changes: 26 additions & 6 deletions src/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,13 +825,18 @@ pub fn isolate_add_with(agent: &str, profile: &str, shallow: bool, json: bool) {
// Copy auth files from vault profile
activate_into(agent, profile, &dir);

// Store metadata
// Store metadata. Without it, read_isolate_mode() later can't tell whether
// this isolate is shallow or deep, so a failed write must not pass silently.
let meta = serde_json::json!({"mode": if shallow { "shallow" } else { "deep" }, "agent": agent, "profile": profile});
fs::write(
if let Err(e) = fs::write(
dir.join("isolate.json"),
serde_json::to_string_pretty(&meta).unwrap() + "\n",
)
.ok();
) {
eprintln!(
"warning: failed to write isolate metadata to {}: {e}",
dir.display()
);
}

if json {
println!(
Expand Down Expand Up @@ -1072,12 +1077,27 @@ fn activate_into(agent: &str, profile: &str, target_dir: &std::path::Path) {

#[cfg(not(windows))]
fn symlink_or_copy(src: &std::path::Path, dest: &std::path::Path) {
std::os::unix::fs::symlink(src, dest).ok();
warn_on_link_failure(src, dest, std::os::unix::fs::symlink(src, dest));
}

#[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(|_| ()));
Comment on lines 1083 to +1085

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 || true

Repository: 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 || true

Repository: getappz/agentflare

Length of output: 797

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '760,835p' src/auth.rs

Repository: 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.

}

/// An already-present destination is fine (the isolate was set up before); any
/// other failure means the isolate is missing host state (ssh keys, git config)
/// it needs, so surface it instead of silently continuing.
fn warn_on_link_failure(src: &std::path::Path, dest: &std::path::Path, res: std::io::Result<()>) {
if let Err(e) = res
&& e.kind() != std::io::ErrorKind::AlreadyExists
{
eprintln!(
"warning: failed to link {} into isolate at {}: {e}",
src.display(),
dest.display()
);
}
}

#[cfg(test)]
Expand Down
Loading