Conversation
Split single-file src/update.rs into src/update/{mod,github,swap}.rs.
swap.rs is the reusable MCP-safe binary-replacement primitive:
- never signals/kills any process, so a live `agentflare mcp` server keeps
running its loaded image and picks up the new binary on next launch
- Windows: rename-aside + copy, with rollback on failure and a deferred
.bat fallback when the exe is hard-locked against renaming
- Unix: same-fs staged atomic rename
- find_killable_pids() reports (never kills) other running instances
github.rs isolates release discovery/download/checksum, with pure
parse_checksum/verify_checksum helpers now unit-tested.
Prepares update::swap::replace_binary for reuse by dev-install (#127).
…nary New `agentflare dev-install` subcommand: cargo build (release by default), resolve the built binary via `cargo metadata` (honors CARGO_TARGET_DIR and workspace shared targets), verify it answers `--version` under a timeout, then swap it over the running binary using the MCP-safe update::swap::replace_binary primitive (#122) — so a live `agentflare mcp` server is never disturbed. - refuses to overwrite the build output itself (guards against running from the freshly built binary instead of the installed one) - --debug builds debug; --dry-run reports the swap without performing it - pure cargo-metadata target-dir parsing + same-file guard are unit-tested
…ofile> CodeRabbit (PR #206): built_binary_path assumed target_directory/<profile>/, which is wrong when build.target / CARGO_BUILD_TARGET adds a <triple>/ segment (or a custom profile renames the dir) -- the lookup would miss the binary. Read the executable path from cargo's compiler-artifact JSON message instead of reconstructing it, so any target/profile layout is handled. build_and_locate() replaces the target-dir/profile-dir reconstruction and its parser.
… net timeout PR #206 CodeRabbit findings on the #122 swap/github code: - swap.rs: surface Windows rollback failure (and the recoverable old-binary path) instead of swallowing it, honoring the "never leave without a binary" invariant. - swap.rs: pid-scope the Unix staging filename so concurrent swaps against the same target cannot clobber each other's staged file. - github.rs: give the release HTTP client connect + per-read timeouts so a hung network cannot block an update indefinitely (no overall timeout, to avoid capping large asset downloads).
…ead error CodeRabbit (PR #206) on build_and_locate: - Reject a non-host CARGO_BUILD_TARGET early with a clear message: dev-install replaces the running binary, so a cross target would build something that cannot run here. verify_runs() remains the backstop for a .cargo/config build.target. - Always child.wait() even when reading cargo stdout fails, so a failed read never orphans the cargo process.
📝 WalkthroughWalkthroughAdds ChangesDevelopment installation workflow
Release update workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/dev_install/mod.rs`:
- Around line 99-102: Update the timeout branch in the verification child flow
to wait for the process after calling child.kill() and before returning the
"--version timed out" error, ensuring the killed child is reaped while
preserving the existing timeout result.
In `@src/update/github.rs`:
- Around line 80-88: Update check_for_update to parse latest and current as
semantic versions and return Some(latest) only when latest is greater than
current, avoiding downgrades when the running build is newer. Preserve the
existing no-update result otherwise, and add tests covering a newer current
version and prerelease comparisons.
- Around line 13-35: The asset-name flow around target_triple, asset_name, and
the unconditional run() call must reject unsupported platforms before
constructing a release filename. Replace the empty-string fallback with an
explicit unsupported-target result or validate target_triple before asset_name
formats the name, and surface a clear platform error from run() while preserving
existing names for supported targets.
In `@src/update/mod.rs`:
- Around line 97-102: Update the no-update branch in github::check_for_update
handling to print the “agentflare is up to date” message only when quiet mode is
not enabled, while still returning None in all cases.
- Around line 127-155: Update extract_binary to create an owned randomized
tempfile::TempDir instead of the predictable PID-based directory, and retain
that TempDir through extraction and replacement. Change its return value and the
corresponding replace_binary flow so the extracted binary path remains valid
until copying completes, while preserving archive validation and cleanup via
TempDir ownership.
In `@src/update/swap.rs`:
- Around line 107-120: Update the swap logic around the generated script and its
caller to stop embedding staged and target filesystem paths in batch source.
Pass both paths as native arguments to a helper executable, preserving the
existing wait-for-pid, copy, cleanup, and self-removal behavior without allowing
path characters such as % to be interpreted by the shell.
- Around line 110-126: The deferred updater script must not delete the staged
binary unless the copy succeeds, and must retry or preserve it when replacement
remains blocked. Update the deferred replacement flow around replace_binary and
its generated batch script to report Scheduled separately from Replaced, so
callers do not claim an immediate successful update when only a deferred attempt
was launched.
- Around line 76-85: Update the error branch handling the failed binary copy in
the swap flow to remove the partially created target before calling
std::fs::rename(&old, target) for rollback. Preserve the existing rollback error
reporting and successful rollback response after cleanup.
🪄 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: fe2c103c-cfc6-4e4f-8c91-ae0e73b99e73
📒 Files selected for processing (9)
src/cli/dev_install.rssrc/cli/mod.rssrc/dev_install/cargo.rssrc/dev_install/mod.rssrc/main.rssrc/update.rssrc/update/github.rssrc/update/mod.rssrc/update/swap.rs
💤 Files with no reviewable changes (1)
- src/update.rs
| if Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| return Err("--version timed out".to_string()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reap the verification child after timing out.
kill() is followed by an immediate return, leaving the child unreaped. Wait after killing it before returning the timeout error.
Proposed fix
if Instant::now() >= deadline {
let _ = child.kill();
+ let _ = child.wait();
return Err("--version timed out".to_string());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if Instant::now() >= deadline { | |
| let _ = child.kill(); | |
| return Err("--version timed out".to_string()); | |
| } | |
| if Instant::now() >= deadline { | |
| let _ = child.kill(); | |
| let _ = child.wait(); | |
| return Err("--version timed out".to_string()); | |
| } |
🤖 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/dev_install/mod.rs` around lines 99 - 102, Update the timeout branch in
the verification child flow to wait for the process after calling child.kill()
and before returning the "--version timed out" error, ensuring the killed child
is reaped while preserving the existing timeout result.
| pub(crate) fn target_triple() -> &'static str { | ||
| if cfg!(all(target_os = "linux", target_arch = "x86_64")) { | ||
| "x86_64-unknown-linux-gnu" | ||
| } else if cfg!(all(target_os = "linux", target_arch = "aarch64")) { | ||
| "aarch64-unknown-linux-gnu" | ||
| } else if cfg!(all(target_os = "macos", target_arch = "x86_64")) { | ||
| "x86_64-apple-darwin" | ||
| } else if cfg!(all(target_os = "macos", target_arch = "aarch64")) { | ||
| "aarch64-apple-darwin" | ||
| } else if cfg!(all(target_os = "windows", target_arch = "x86_64")) { | ||
| "x86_64-pc-windows-msvc" | ||
| } else { | ||
| "" | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn asset_ext() -> &'static str { | ||
| if cfg!(windows) { "zip" } else { "tar.gz" } | ||
| } | ||
|
|
||
| pub(crate) fn asset_name(_version: &str) -> String { | ||
| format!("agentflare-{}.{}", target_triple(), asset_ext()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/update/github.rs --view expanded
rg -n "target_triple|asset_name|unsupported platform|unsupported|platform" src/update -S
sed -n '1,220p' src/update/github.rsRepository: getappz/agentflare
Length of output: 8094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- outline ---\n'
ast-grep outline src/update/github.rs --view expanded
printf '\n--- search ---\n'
rg -n "target_triple|asset_name|unsupported platform|unsupported|platform" src/update -S
printf '\n--- file ---\n'
sed -n '1,220p' src/update/github.rsRepository: getappz/agentflare
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/update/github.rs --view expanded
rg -n "target_triple|asset_name|unsupported platform|unsupported|platform" src/update -S
sed -n '1,220p' src/update/github.rsRepository: getappz/agentflare
Length of output: 8094
🏁 Script executed:
sed -n '1,220p' src/update/mod.rsRepository: getappz/agentflare
Length of output: 5530
🏁 Script executed:
sed -n '120,180p' src/update/mod.rsRepository: getappz/agentflare
Length of output: 2002
🏁 Script executed:
sed -n '1,60p' src/update/mod.rs && printf '\n---\n' && sed -n '130,170p' src/update/mod.rsRepository: getappz/agentflare
Length of output: 3366
Reject unsupported targets before constructing the release asset name
run() calls github::asset_name() unconditionally, and target_triple() returns "" on unsupported targets. That produces agentflare-.<ext> and pushes the failure into the download path instead of surfacing a clear platform error.
🤖 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/update/github.rs` around lines 13 - 35, The asset-name flow around
target_triple, asset_name, and the unconditional run() call must reject
unsupported platforms before constructing a release filename. Replace the
empty-string fallback with an explicit unsupported-target result or validate
target_triple before asset_name formats the name, and surface a clear platform
error from run() while preserving existing names for supported targets.
| pub(crate) fn check_for_update() -> Result<Option<String>, String> { | ||
| let latest = latest_version()?; | ||
| let current = format!("v{}", env!("CARGO_PKG_VERSION")); | ||
| if latest != current { | ||
| Ok(Some(latest)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare semantic versions rather than treating every mismatch as an update.
If the running build is newer than GitHub’s latest release, this returns Some(latest) and downgrades it. Parse both versions and update only when latest > current; add newer-current and prerelease tests.
🤖 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/update/github.rs` around lines 80 - 88, Update check_for_update to parse
latest and current as semantic versions and return Some(latest) only when latest
is greater than current, avoiding downgrades when the running build is newer.
Preserve the existing no-update result otherwise, and add tests covering a newer
current version and prerelease comparisons.
| let latest = match github::check_for_update() { | ||
| Ok(Some(v)) => v, | ||
| Ok(None) => { | ||
| println!("agentflare is up to date (v{})", env!("CARGO_PKG_VERSION")); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor --quiet when no update is available.
The up-to-date message is currently unconditional.
Proposed fix
Ok(Some(v)) => v,
Ok(None) => {
- println!("agentflare is up to date (v{})", env!("CARGO_PKG_VERSION"));
+ if !quiet {
+ println!("agentflare is up to date (v{})", env!("CARGO_PKG_VERSION"));
+ }
return None;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let latest = match github::check_for_update() { | |
| Ok(Some(v)) => v, | |
| Ok(None) => { | |
| println!("agentflare is up to date (v{})", env!("CARGO_PKG_VERSION")); | |
| return None; | |
| } | |
| let latest = match github::check_for_update() { | |
| Ok(Some(v)) => v, | |
| Ok(None) => { | |
| if !quiet { | |
| println!("agentflare is up to date (v{})", env!("CARGO_PKG_VERSION")); | |
| } | |
| return None; | |
| } |
🤖 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/update/mod.rs` around lines 97 - 102, Update the no-update branch in
github::check_for_update handling to print the “agentflare is up to date”
message only when quiet mode is not enabled, while still returning None in all
cases.
| fn extract_binary(data: &[u8]) -> Result<std::path::PathBuf, String> { | ||
| let tmpdir = std::env::temp_dir().join(format!("agentflare-update-{}", std::process::id())); | ||
| let _ = std::fs::remove_dir_all(&tmpdir); | ||
| std::fs::create_dir_all(&tmpdir).map_err(|e| format!("create tmpdir: {e}"))?; | ||
|
|
||
| match github::asset_ext() { | ||
| "tar.gz" => { | ||
| let tar_gz = flate2::read::GzDecoder::new(data); | ||
| let mut archive = tar::Archive::new(tar_gz); | ||
| archive | ||
| .unpack(&tmpdir) | ||
| .map_err(|e| format!("extract error: {e}"))?; | ||
| } | ||
| "zip" => { | ||
| let cursor = std::io::Cursor::new(data); | ||
| let mut archive = | ||
| zip::ZipArchive::new(cursor).map_err(|e| format!("zip error: {e}"))?; | ||
| archive | ||
| .extract(&tmpdir) | ||
| .map_err(|e| format!("extract error: {e}"))?; | ||
| } | ||
| other => return Err(format!("unsupported asset extension: {other}")), | ||
| } | ||
|
|
||
| let new_binary = tmpdir.join(github::binary_name()); | ||
| if !new_binary.exists() { | ||
| return Err("binary not found in archive".to_string()); | ||
| } | ||
| Ok(new_binary) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the update module around the extraction function and its callers.
file="src/update/mod.rs"
wc -l "$file"
sed -n '1,260p' "$file"
printf '\n--- call sites ---\n'
rg -n "extract_binary\\(|github::binary_name\\(|temp_dir\\(|remove_dir_all\\(" src/update -nRepository: getappz/agentflare
Length of output: 6323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, tempfile, shutil, pathlib
base = tempfile.mkdtemp(prefix="probe-")
try:
target = pathlib.Path(base) / "target"
link = pathlib.Path(base) / "link"
target.mkdir()
link.symlink_to(target, target_is_directory=True)
print("link exists:", link.exists())
print("link is symlink:", link.is_symlink())
print("link points to dir:", link.resolve().is_dir())
# Probe mkdir-on-symlink behavior analogous to create_dir_all on an existing path.
try:
os.makedirs(link, exist_ok=True)
print("os.makedirs(link, exist_ok=True): OK")
except Exception as e:
print("os.makedirs(link, exist_ok=True):", type(e).__name__, e)
# Probe mkdir of a child through the symlink.
child = link / "child"
try:
os.makedirs(child, exist_ok=True)
print("os.makedirs(link/child, exist_ok=True): OK")
print("child landed under target:", (target / "child").exists())
except Exception as e:
print("os.makedirs(link/child, exist_ok=True):", type(e).__name__, e)
finally:
shutil.rmtree(base, ignore_errors=True)
PYRepository: getappz/agentflare
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/update/swap.rs"
wc -l "$file"
sed -n '1,260p' "$file"
printf '\n--- relevant symbols ---\n'
rg -n "replace_binary|copy|rename|symlink|open\(" src/update/swap.rsRepository: getappz/agentflare
Length of output: 12103
Use a randomized private tempdir for extraction
agentflare-update-<pid> is predictable in the shared temp directory, so a local process can race extraction or replace the unpacked file before replace_binary copies it. Keep an owned TempDir alive through the swap instead of returning a bare PathBuf.
🤖 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/update/mod.rs` around lines 127 - 155, Update extract_binary to create an
owned randomized tempfile::TempDir instead of the predictable PID-based
directory, and retain that TempDir through extraction and replacement. Change
its return value and the corresponding replace_binary flow so the extracted
binary path remains valid until copying completes, while preserving archive
validation and cleanup via TempDir ownership.
| Err(e) => { | ||
| // Never leave the install without a binary: put the old one back. | ||
| if let Err(re) = std::fs::rename(&old, target) { | ||
| return Err(format!( | ||
| "copy new binary failed ({e}); rollback also failed ({re}); \ | ||
| previous binary preserved at {}", | ||
| old.display() | ||
| )); | ||
| } | ||
| Err(format!("copy new binary: {e}")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the partial target before attempting rollback.
std::fs::copy may create or truncate target before failing. The subsequent rename then fails because that destination exists, leaving the installation corrupted despite the rollback path.
Proposed fix
Err(e) => {
// Never leave the install without a binary: put the old one back.
+ let _ = std::fs::remove_file(target);
if let Err(re) = std::fs::rename(&old, target) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Err(e) => { | |
| // Never leave the install without a binary: put the old one back. | |
| if let Err(re) = std::fs::rename(&old, target) { | |
| return Err(format!( | |
| "copy new binary failed ({e}); rollback also failed ({re}); \ | |
| previous binary preserved at {}", | |
| old.display() | |
| )); | |
| } | |
| Err(format!("copy new binary: {e}")) | |
| Err(e) => { | |
| // Never leave the install without a binary: put the old one back. | |
| let _ = std::fs::remove_file(target); | |
| if let Err(re) = std::fs::rename(&old, target) { | |
| return Err(format!( | |
| "copy new binary failed ({e}); rollback also failed ({re}); \ | |
| previous binary preserved at {}", | |
| old.display() | |
| )); | |
| } | |
| Err(format!("copy new binary: {e}")) |
🤖 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/update/swap.rs` around lines 76 - 85, Update the error branch handling
the failed binary copy in the swap flow to remove the partially created target
before calling std::fs::rename(&old, target) for rollback. Preserve the existing
rollback error reporting and successful rollback response after cleanup.
| let script = format!( | ||
| "@echo off\r\n\ | ||
| :wait\r\n\ | ||
| tasklist /FI \"PID eq {pid}\" 2>nul | find \"{pid}\" >nul && (\r\n\ | ||
| ping -n 2 127.0.0.1 >nul\r\n\ | ||
| goto wait\r\n\ | ||
| )\r\n\ | ||
| copy /y \"{staged}\" \"{target}\" >nul\r\n\ | ||
| del \"{staged}\" >nul 2>&1\r\n\ | ||
| del \"%~f0\" >nul 2>&1\r\n", | ||
| pid = pid, | ||
| staged = staged.display(), | ||
| target = target.display(), | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not interpolate filesystem paths into a batch program.
Quoted batch strings still expand characters such as %, allowing specially named installation/temp paths to alter commands. Pass paths as native process arguments to a helper executable instead of generating shell source.
🤖 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/update/swap.rs` around lines 107 - 120, Update the swap logic around the
generated script and its caller to stop embedding staged and target filesystem
paths in batch source. Pass both paths as native arguments to a helper
executable, preserving the existing wait-for-pid, copy, cleanup, and
self-removal behavior without allowing path characters such as % to be
interpreted by the shell.
Source: Linters/SAST tools
| tasklist /FI \"PID eq {pid}\" 2>nul | find \"{pid}\" >nul && (\r\n\ | ||
| ping -n 2 127.0.0.1 >nul\r\n\ | ||
| goto wait\r\n\ | ||
| )\r\n\ | ||
| copy /y \"{staged}\" \"{target}\" >nul\r\n\ | ||
| del \"{staged}\" >nul 2>&1\r\n\ | ||
| del \"%~f0\" >nul 2>&1\r\n", | ||
| pid = pid, | ||
| staged = staged.display(), | ||
| target = target.display(), | ||
| ); | ||
| std::fs::write(&bat, script).map_err(|e| format!("write deferred updater: {e}"))?; | ||
| std::process::Command::new("cmd") | ||
| .args(["/C", "start", "/min", "", &bat.to_string_lossy()]) | ||
| .spawn() | ||
| .map_err(|e| format!("spawn deferred updater: {e}"))?; | ||
| Ok(()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not delete the staged binary after an unchecked deferred copy.
The script suppresses copy failures, then deletes both the staged binary and itself, while replace_binary has already returned success. A persistent lock or permissions failure therefore loses the update and still causes the caller to print “updated.” Preserve/retry on failure and distinguish Replaced from Scheduled.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 121-122: Passing non-literal (user-controlled or interpolated) data to a shell invoked via std::process::Command::new("sh"|"bash"|...) with -c allows command injection. Avoid spawning a shell: pass the program and each argument separately to Command::new(program).arg(arg) so the OS never re-parses the string, or strictly allowlist/escape any value that must reach a shell.
Context: std::process::Command::new("cmd")
.args(["/C", "start", "/min", "", &bat.to_string_lossy()])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-process-shell-rust)
🤖 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/update/swap.rs` around lines 110 - 126, The deferred updater script must
not delete the staged binary unless the copy succeeds, and must retry or
preserve it when replacement remains blocked. Update the deferred replacement
flow around replace_binary and its generated batch script to report Scheduled
separately from Replaced, so callers do not claim an immediate successful update
when only a deferred attempt was launched.
Auto-opened on
item donefor 019f69ee-2317-7892-bd43-0e4a97bb9666.Summary by CodeRabbit
New Features
dev-installto build and install the current source, with debug and dry-run options.Improvements